Skip to content
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

Allow space-separated passenv variables #2628

Closed
wants to merge 1 commit into from

Conversation

masenf
Copy link
Collaborator

@masenf masenf commented Dec 8, 2022

Fixes #2615

Thanks for contribution

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_lint)
  • wrote descriptive pull request text
  • ensured there are test(s) validating the fix
  • added news fragment in docs/changelog folder
  • updated/extended the documentation

@masenf
Copy link
Collaborator Author

masenf commented Dec 8, 2022

I think this might break the comma separated case 🤔, but it's my first stab.

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.

This will not work, though. This is hacking it into, rather than supported by design. tox 4 no longer wants to be in the business of custom parsers per key that confuse new users. Any parsing logic needs to apply to all keys and must be done at the loader level, not in post-processing. The post-processing is reserved for transforming values to other types, not hacking in a custom parsing logic. Thanks for the try, though.

Comment on lines 134 to -135
values.extend(self._default_pass_env())
return sorted({k: None for k in values}.keys())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No operation might happen here because this is past the INI file loader: translation this will break a soon-to-add native pyproject.toml config intuitive behavior, which I'm not willing to put up with. You can't make changes to the INI file loader because that applies to all other keys, and e.g. for deps you don't want this. Hence my comment, this can't be done clearly, so I'm willing to make a breaking change.

@gaborbernat
Copy link
Member

Closing per my comment above.

@gaborbernat gaborbernat closed this Dec 8, 2022
@masenf
Copy link
Collaborator Author

masenf commented Dec 8, 2022

Thanks for the review @gaborbernat, still trying to wrap my head around the tox4 world, which is totally awesome!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

passenv regression when multiple variables are named on the same line
2 participants