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

Fix substitution problems introduced in 2.8 #599

Merged
merged 6 commits into from Sep 4, 2017
Merged

Fix substitution problems introduced in 2.8 #599

merged 6 commits into from Sep 4, 2017

Conversation

@obestwalter
Copy link
Member

@obestwalter obestwalter commented Sep 3, 2017

fixes #595

The way #515 was implemented did not play nicely with env var substitution. This might render the former additions completely or partially obsolete but I did not check this yet. First let's fix that bug.

Instead of crashing early on testenv creation if substitution key is missing, the missing keys are added to a list attached to TestenvConfig. This way it is not necessary anymore to provide everything needed to resolve all testenvs but instead only what is needed to resolve all testenvs that are part of this restrun.

Implementation notes:

The first implementation used a module level mapping from envname to missing substitutions, which was ugly. Instead the information is bubbled up as an exception where it can be handled the way it is needed in the contexts where the information can be either translated into a crashing error or attached to the object which should carry the information (in this case TestenvConfig).

Atm a missing substitution in a testenv is not being left empty but filled with a special value 'TOX_MISSING_SUBSTITUTION', this is not exactly necessary, but I don't feel comfortable enough yet to leave it empty, this can help with debugging problems that might arise from this change.

obestwalter added 4 commits Sep 2, 2017
The way #515 was implemented did not play nicely with env var substitution. This might render the former additions completely or partially obsolete but I did not check this yet. First let's fix that bug.
(cherry picked from commit e902f04)
The way #515 was implemented did not play nicely with env var substitution. This might render the former additions completely or partially obsolete but I did not check this yet. First let's fix that bug.

Instead of crashing early on testenv creation if the substitution the missing substitution keys are added to a list attached to TestenvConfig. This way it is not necessary anymore to provide everything needed to resolve all testenvs but instead only what is needed to resolve all testenvs that are part of this restrun.

Implementation notes:

The first implementation used a module level mapping from envname to missing substitutions, which was ugly. Instead the information is bubbled up as an exception where it can be handled the way it is needed in the contexts where the information can be either translated into a crashing error or attached to the object which should carry the information (in this case TestenvConfig).

Atm a missing substitution in a testenv is not being left empty but filled with a special value 'TOX_MISSING_SUBSTITUTION', this is not exactly necessary, but I don't feel comfortable enough yet to leave it empty, this can help with debugging potential problems that might arise from this change.
Crash only if actually trying to run a testenv with missing substitutions
@obestwalter obestwalter changed the title #595 Fix substitution problems introduced in 2.8 Sep 3, 2017
@obestwalter obestwalter mentioned this pull request Sep 3, 2017
@obestwalter
Copy link
Member Author

@obestwalter obestwalter commented Sep 3, 2017

Well, this is still not quite doing what it should, so stay tuned :)

On the plus side: this is actually the first time that I spent some decent quality time with the actual code and start to understand how things play together on the configuration level.

obestwalter added 2 commits Sep 3, 2017
Instead of crashing the whole thing again - only later, do it the right way by setting venv.status with the error to be reported later.
* catch MissingSubstition from all method calls for consistency
* set status and stop execution in setupvenv if substutions are missing to prevent errors occuring there with missing substitutions
@obestwalter
Copy link
Member Author

@obestwalter obestwalter commented Sep 3, 2017

This should be it.

@obestwalter
Copy link
Member Author

@obestwalter obestwalter commented Sep 4, 2017

Thanks @Adaephon-GH for an ad-hoc real life code review 🤗 I think we're good to go.

@obestwalter obestwalter merged commit b56738c into tox-dev:master Sep 4, 2017
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@obestwalter obestwalter deleted the Avira:#595 branch Sep 4, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

1 participant