resolve #6595 use OO inheritence in activate.py #7049
Conversation
There was a problem hiding this comment.
The shell-specific code in build_activate and _update_prompt should go into the appropriate derived classes (https://github.com/conda/conda/pull/7049/files#diff-a871ac4214094e0e18e2c240a36dcab3R214 and https://github.com/conda/conda/pull/7049/files#diff-a871ac4214094e0e18e2c240a36dcab3R399).
Otherwise, LGTM for now.
| def __init__(self, shell, arguments=None): | ||
| self.shell = shell | ||
| # The following instance variables must be defined by each implementation. | ||
| shell = None |
There was a problem hiding this comment.
Technically, we don't need self.shell anymore and thus could remove it.
|
|
||
| try: | ||
| from cytoolz.itertoolz import concatv, drop | ||
| except ImportError: # pragma: no cover | ||
| from ._vendor.toolz.itertoolz import concatv, drop # NOQA | ||
|
|
||
|
|
||
| class Activator(object): | ||
| class _Activator(object): |
There was a problem hiding this comment.
We could make this class _Activator(ABC) and thus use @abstractmethod to automatically check for methods/properties being implemented.
This would add a little bit more boilerplate (which I would be fine with). But we would also need to look out for Python 2 intricacies (e.g., need to use metaclass=ABCMeta, the deprecated @abstractproperty, ...).
So, although I'd prefer using ABC, I'm also not eager to deal with PY2 stuff...
There was a problem hiding this comment.
Yeah that's why I didn't go in that direction, and just renamed Activator to _Activator.
| shift_args = None | ||
| command_join = None | ||
|
|
||
| unset_var_tmpl = None |
There was a problem hiding this comment.
We could replace those template with methods that return the appropriate formatted strings, like suggested in #6585 (comment), which would make the code a little cleaner/better readable.
But that could also be done in a subsequent PR anytime and is not crucial for this one.
|
@mbargull If you're around, do we look good to go here now? |
|
LGTM. Saw that you already fixed |
|
Hi there, thank you for your contribution to Conda! This pull request has been automatically locked since it has not had recent activity after it was closed. Please open a new issue or pull request if needed. |
resolve #6595