resolve #6733 make empty env vars sequence-safe for sequence parameters #6741
Conversation
kalefranz
force-pushed
the
resolve-6733-empty-env-var-config
branch
from
3b6dfcb
to
a9981e1
Compare
conda/common/configuration.py
Outdated
| @@ -210,9 +210,9 @@ def value(self, parameter_obj): | |||
| # TODO: add stripping of !important, !top, and !bottom | |||
| raw_value = self._raw_value | |||
| if string_delimiter in raw_value: | |||
There was a problem hiding this comment.
self._raw_value is always a str, right?
str.split always returns a list of strings so that you can simplify the whole if-else-block to
value = [v for v in self._raw_value.split(string_delimiter) if v]. If you want to allow empty strings anywhere in the sequence, you could use a trailing separator to distinguish ""->[] from ","->[""] with
value = self._raw_value.split(string_delimiter) # str.split always returns a list with length >=1
value = value if value[-1] else value[:-1]There was a problem hiding this comment.
self._raw_value is always a str, right?
I guess in this case that's safe to assume. I feel like I'd want to add an assert in there though just to be safe.
If you want to allow empty strings anywhere in the sequence
Can you think of a use-case that we currently have, or one that we could potentially have, where you'd ever want an empty string? I can't, at the moment.
There was a problem hiding this comment.
The currently available SequenceParameters in base/context.py are lists of packages (specs), features, paths (/subdirs), or channels.
So, no real use case there. Hence this would be more of a "possible forward-compatibility" decision.
There was a problem hiding this comment.
I say for now that we don't support empty strings. I can see it just as easily causing us problems in the future, as it could saving us from a future problem.
There was a problem hiding this comment.
That's fine by me. My mind is oscillating between 30/70, 50/50, and 70/30 on whether the one or the other possibility would cause more potential issues. Currently I'm slightly more leaning toward removing empty strings, just like you did 😉. So, be fast and merge before I change my mind again and again 😝
kalefranz
force-pushed
the
resolve-6733-empty-env-var-config
branch
from
a2a6abe
to
37cbb36
Compare
|
Unit test added! |
|
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 #6733
@mbargull Let's see how this works... I don't even know if there's a situation where we'd actually want an empty string, instead of just an empty sequence.