Skip to content

Conversation

realitycheck
Copy link
Contributor

@realitycheck realitycheck commented Apr 29, 2025

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)! -->

  • [+ ] ran the linter to address style issues (tox -e fix)
  • [+] wrote descriptive pull request text
  • [+] ensured there are test(s) validating the fix
  • added news fragment in docs/changelog folder
  • updated/extended the documentation

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):
Copy link
Member

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)
Copy link
Member

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?

Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

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. 🤷

Copy link
Member

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?

@realitycheck realitycheck force-pushed the main branch 2 times, most recently from 2ed59d3 to 9cc6076 Compare May 12, 2025 07:47
Copy link
Member

@gaborbernat gaborbernat left a 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 😊

@realitycheck realitycheck requested a review from gaborbernat May 13, 2025 08:20
@gaborbernat gaborbernat merged commit 957f2f8 into tox-dev:main May 13, 2025
27 of 28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants