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

Revert "Warn about commands key missing in tox.ini" #456

Merged
merged 2 commits into from
Feb 24, 2017
Merged

Revert "Warn about commands key missing in tox.ini" #456

merged 2 commits into from
Feb 24, 2017

Conversation

asottile
Copy link
Contributor

Reverts #407

See #454 and #316

I'd rather fix this to have the following behaviour but at a glance it doesn't seem easy / possible with the current argvlist parser (which doesn't differentiate between missing and present-but-empy):

  • If there are no commands: warning (or error)
  • If commands is present but empty: successfully do nothing (no warning).

CHANGELOG Outdated
@@ -59,9 +59,6 @@
- fix #399. Make sure {envtmpdir} is created if it doesn't exist at the
start of a testenvironment run. Thanks Manuel Jacob.

- fix #316: Lack of commands key in ini file is now treated as an error.
Reported virtualenv status is 'nothing to do' instead of 'commands
succeeded', with relevant error message displayed. Thanks Lukasz Rogalski.

Copy link
Member

@obestwalter obestwalter Feb 17, 2017

Choose a reason for hiding this comment

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

The changelog must not be changed retroactively, so this should not be removed. Instead a new entry for for the current version needs to be added, that this fix did not what was intended to do (throw a warning instead of causing an error) .

@asottile
Copy link
Contributor Author

Nice catch! I've preserved + updated the changelog

@obestwalter obestwalter merged commit de42213 into tox-dev:master Feb 24, 2017
@asottile asottile deleted the revert-407-warn-about-commands-missing branch February 24, 2017 16:34
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.

2 participants