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 {posargs} in setenv #1697

Merged
merged 2 commits into from
Oct 18, 2020

Conversation

jayvdb
Copy link

@jayvdb jayvdb commented Oct 18, 2020

Closes #1695

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)

Escape {packages} and {opts} so they are literals
allowed only in `install_command`.
@jayvdb jayvdb force-pushed the rm-install-command-special-cases branch from 08ee204 to ad3d69f Compare October 18, 2020 16:56
@jayvdb jayvdb force-pushed the rm-install-command-special-cases branch from ad3d69f to d3ef0b4 Compare October 18, 2020 16:59
@jayvdb
Copy link
Author

jayvdb commented Oct 18, 2020

The changes re {packages} and {opts} doesnt radically improve them - they were errors (backtraces) before if they were used outside install_command, and they will still be backtraces, but now the error is the same as any other unknown substitution key. As a result there isnt any tests in the PR for this, nor any changelog mention because the user doesnt see anything. I'm happy to add it to the changelog entry anyway if it is strictly necessary to note any invisible/internal change.

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 e4d0d60 into tox-dev:master Oct 18, 2020
@jayvdb jayvdb mentioned this pull request Jan 11, 2021
# special case: opts and packages. Leave {opts} and
# {packages} intact, they are replaced manually in
# _venv.VirtualEnv.run_install_command.
if sub_value in ("opts", "packages"):
Copy link
Author

Choose a reason for hiding this comment

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

This removal caused #1777

continue
elif word.startswith("{posargs:") and word.endswith("}"):
Copy link
Author

Choose a reason for hiding this comment

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

This case wasn't handled correctly in the new code, if : was in the middle of this text range, resulting in regression #1785

ssbarnea pushed a commit to ssbarnea/tox that referenced this pull request Apr 19, 2021
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.

{packages}, {opts} and {posargs} in substititions in wrong context has lots of odd backtraces
2 participants