-
-
Notifications
You must be signed in to change notification settings - Fork 539
Fix for tox4 regression issue with setenv file and substitutions (#2435) #3521
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
src/tox/config/set_env.py
Outdated
if not self._is_file_line(sub_line): | ||
sub_raw.__setitem__(*self._extract_key_value(sub_line)) | ||
else: | ||
for k, v in self._stream_env_file(self._parse_file_line(sub_line), args): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No single letter variables outside of comprehensions.
line = self._needs_replacement.pop(0) | ||
expanded_line = self._replacer(line, ConfigLoadArgs([], self._name, self._env_name)) | ||
sub_raw = dict(self._extract_key_value(sub_line) for sub_line in expanded_line.splitlines() if sub_line) | ||
expanded_line = self._replacer(line, args) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain the change here and why you done it this way?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- [+] wrote descriptive pull request text
This part you didn't really address. Writing a descriptive text involves the approach you've taken and why.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that's ConfigLoadArgs
variable is being used twice now since the call to _stream_env_file(_read_env_file) requires it. Seems like no point in instantiating it twice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, the description is pointing out to the long standing regression issue that wasn't fixed yet, this is an attempt to do it, since it was stated there "PR welcome to fix it.", what else could be said. 🤷
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that's
ConfigLoadArgs
variable is being used twice now since the call to _stream_env_file(_read_env_file) requires it
Was looking for an explanation for this, as in why we need to call it twice now?
2ed59d3
to
9cc6076
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's add changelog entry 😊
for more information, see https://pre-commit.ci
This PR contains fix for the issue #2435. (#2435 (comment))
Please, make sure you address all the checklists (for details on how see
development documentation)! -->
tox -e fix
)docs/changelog
folder