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.

Sign up for GitHub

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

Jump to bottom

Fix escaping brackets in setenv #1691

Merged
merged 3 commits into from
Oct 21, 2020
Merged

Fix escaping brackets in setenv #1691

merged 3 commits into from
Oct 21, 2020

Conversation

jayvdb
Copy link

@jayvdb jayvdb commented Oct 17, 2020 •
edited
Loading

Fixs regression in v3.20.0 that caused escaped curly braces in setenv to break usage of the variable elsewhere in tox.ini.

Fixes #1690

Contribution checklist:

(also see CONTRIBUTING.rst for details)

  • wrote descriptive pull request text
  • added/updated test(s)
  • updated/extended the documentation
  • added relevant issue keyword
    in message body
  • added news fragment in changelog folder
    • fragment name: <issue number>.<type>.rst for example (588.bugfix.rst)
    • <type> is must be one of bugfix, feature, deprecation,breaking, doc, misc
    • if PR has no issue: consider creating one first or change it to the PR number after creating the PR
    • "sign" fragment with "by :user:<your username>"
    • please use full sentences with correct case and punctuation, for example: "Fix issue with non-ascii contents in doctest text files - by :user:superuser."
    • also see examples
  • added yourself to CONTRIBUTORS (preserving alphabetical order)

@jayvdb jayvdb force-pushed the fix-escape-bracket branch from c076123 to c029347 Compare October 17, 2020 13:34
@jayvdb jayvdb force-pushed the fix-escape-bracket branch 3 times, most recently from b2303c2 to afcd915 Compare October 17, 2020 13:43
@jayvdb
Copy link
Author

jayvdb commented Oct 17, 2020

The macOS CI error appears to be unrelated.

asottile
asottile reviewed Oct 17, 2020
View reviewed changes
Copy link
Contributor

@asottile asottile left a comment

Choose a reason for hiding this comment

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

it's unclear to me what this is actually fixing, why would you still want the backslashes in the variable?

can you show a concrete usecase?

src/tox/config/__init__.py Outdated
# such as {} being escaped using \{\}, suitable for use with
# os.environ .
return {
name: re.sub(r"\\({|})", r"\1", value)
Copy link
Contributor

Choose a reason for hiding this comment

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

regex is overkill here, I'd just use .replace -- it looks like the original code did this why did you change it? (the original code is much more readable imo)

Copy link
Author

Choose a reason for hiding this comment

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

I didnt pay attention to the original code, as it was in a very different area of the code exercised by different tests. I'm happy to use .replace.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah please do.

Copy link
Author

Choose a reason for hiding this comment

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

This was done. Please review again when you have time, as this PR merge conflicts with other PRs, and is also a prerequisite for testing in other PRs using brackets in settings and their effect on the runtime environment variables present while running commands.

tests/unit/test_venv.py
assert env["ESCAPED_VAR"] == "{value}"
assert env["ESCAPED_VAR2"] == r"\{value\}"
assert env["COLON"] == ";" if sys.platform == "win32" else ":"
assert env["TTY_VAR"] == "OFF_VALUE"
Copy link
Contributor

Choose a reason for hiding this comment

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

these seem unrelated to the patch

Copy link
Author

Choose a reason for hiding this comment

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

these are testing other parts of substitution internals which are not tested in this context. If export() is to work correctly with a stable API, all of the substitution internal values need to be converted into externally understandable values.

For the record, using {posargs} in setenv is another case of special handling in the substitution system, and it fails strangely in setenv.

And another internal voodoo not tested is file| - I havent tried that yet.

Copy link
Author

Choose a reason for hiding this comment

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

I've tested with file| - no problems here, but #1694 raised for other issues.

I also retested with {posargs}, {packages} and {opts} and very odd behaviour raised in #1695

@jayvdb
Copy link
Author

jayvdb commented Oct 17, 2020

why would you still want the backslashes in the variable?

backslashes are how tox escapes { so that { can be a literal in a variable value.
This is in the docs at https://tox.readthedocs.io/en/latest/config.html#substitutions

If you are asking about literal backslashes in the variable value, there are many reasons people might want them.
https://www.google.com/search?q=backslashes+in+environment+variables

gaborbernat
gaborbernat requested changes Oct 18, 2020
View reviewed changes
src/tox/config/__init__.py Outdated
# such as {} being escaped using \{\}, suitable for use with
# os.environ .
return {
name: re.sub(r"\\({|})", r"\1", value)
Copy link
Member

Choose a reason for hiding this comment

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

Yeah please do.

@jayvdb jayvdb force-pushed the fix-escape-bracket branch 2 times, most recently from 965be74 to e641fa6 Compare October 18, 2020 13:47
jayvdb added 2 commits October 19, 2020 06:40
@jayvdb
Fix test test_curly_braces_in_setenv
6eb24c6 
The internal state of SetenvDict depends on
curly brackets being escaped.
@jayvdb
Revert "Allow escaping curly braces in setenv"
ce109b2 
This partially reverts commit 3c38762.

Fixes tox-dev#1690
@jayvdb jayvdb force-pushed the fix-escape-bracket branch from e641fa6 to 9f356b7 Compare October 18, 2020 23:43
@jayvdb jayvdb force-pushed the fix-escape-bracket branch from 9f356b7 to d62320f Compare October 19, 2020 01:08
@jayvdb jayvdb mentioned this pull request Oct 19, 2020
Closed
6 tasks
gaborbernat
gaborbernat approved these changes Oct 21, 2020
View reviewed changes
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.

@gaborbernat gaborbernat merged commit 8d40707 into tox-dev:master Oct 21, 2020
@jayvdb jayvdb mentioned this pull request Oct 22, 2020
Closed
jayvdb added a commit to jayvdb/tox that referenced this pull request Oct 23, 2020
@jayvdb
Fix escaping brackets in setenv (tox-dev#1691)
c86f144
jayvdb added a commit to jayvdb/tox that referenced this pull request Oct 23, 2020
@jayvdb
Fix escaping brackets in setenv (tox-dev#1691)
6217b23
jayvdb added a commit to jayvdb/tox that referenced this pull request Oct 23, 2020
@jayvdb
Fix escaping brackets in setenv (tox-dev#1691)
b528048
ssbarnea pushed a commit to ssbarnea/tox that referenced this pull request Apr 19, 2021
@jayvdb @ssbarnea
Fix escaping brackets in setenv (tox-dev#1691)
2fd97e7
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@asottile asottile asottile left review comments

@gaborbernat gaborbernat gaborbernat approved these changes

@obestwalter obestwalter Awaiting requested review from obestwalter

Assignees
No one assigned
Labels
bot:chronographer:provided
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Curly braces cannot be escaped in setenv definition since 3.20.0
3 participants
@jayvdb @gaborbernat @asottile