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

Warn user if they forgot commands section #316

Closed
pytoxbot opened this issue Sep 17, 2016 · 18 comments · Fixed by dropseed/sweep#11
Closed

Warn user if they forgot commands section #316

pytoxbot opened this issue Sep 17, 2016 · 18 comments · Fixed by dropseed/sweep#11
Labels

Comments

@pytoxbot
Copy link

@pytoxbot pytoxbot commented Sep 17, 2016

When one forgets to add commands = ... to tox.ini, this is most likely a mistake - however, tox happily runs nothing and tells us the commands succeeded.

I think instead it should output a warning, similar to the yellow "no tests ran" which pytest does.

@obestwalter obestwalter changed the title "Commands succeeded" when no commands are defined Warn user if they forgot commands section Nov 3, 2016
@obestwalter
Copy link
Member

@obestwalter obestwalter commented Nov 3, 2016

@The-Compiler I think this is a good idea to add that. I changed the name of the issue to reflect what should happen rather then what not happens.

@The-Compiler
Copy link
Member

@The-Compiler The-Compiler commented Nov 3, 2016

Cool - thanks for all the issue triaging you're doing lately, it's much appreciated! 😄

@vlcinsky
Copy link

@vlcinsky vlcinsky commented Jan 5, 2017

The ticket talks about WARNing, but the modification is reporting an ERROR and exits with status code 1.

I am often using tox.ini as builder for virtual envs and in those cases I often do not expect any command to be run.

The ERROR message is thus confusing. My first impression was, that the build of virtualenv failed what was not true.

Consider changing this warning only and keep the empty command section as acceptable scenario not reported as a failure.

@obestwalter
Copy link
Member

@obestwalter obestwalter commented Jan 5, 2017

I am often using tox.ini as builder for virtual envs and in those cases I often do not expect any command to be run.

@vlcinsky I agree the commands section must not be mandatory. The issue explicitly states also that we want to warn the user and not throw an error. #407 needs a bit of work as @rogalski already said - I would suggest to print an informative warning about the missing commands section but not throw an error.

@vlcinsky
Copy link

@vlcinsky vlcinsky commented Jan 5, 2017

@obestwalter so we agree.

Using tox v 2.5.0 I get the ERROR reported if command is empty. I wonder, if this issue shall be reopen or I shall file a new one.

@obestwalter
Copy link
Member

@obestwalter obestwalter commented Jan 5, 2017

Let's reopen this. As this made it into a release already I am not sure how to proceed. Let's hear some more opinions about this ...

@obestwalter obestwalter reopened this Jan 5, 2017
@obestwalter
Copy link
Member

@obestwalter obestwalter commented Jan 5, 2017

I just had a look in the docs and there are no mandatory settings in the testenv section. It says explicitly: "Complete list of settings that you can put into testenv* sections:" (emphasis by me). So we should consider this a bug that we now get an error if no commands entry is present.

I also think that it is a valid use case to use tox to only create a virtualenv without executing any commands - I even seem to remember that this is mentioned in the docs somewhere.

@vlcinsky
Copy link

@vlcinsky vlcinsky commented Jan 5, 2017

@obestwalter yes, the page https://tox.readthedocs.io/en/latest/example/devenv.html states:

Tox can be used for just preparing different virtual environments required by a project.

and provides Basic and Advanced example for it. (even showing an option to use empty command = statement.)

@obestwalter
Copy link
Member

@obestwalter obestwalter commented Jan 6, 2017

FWIW it still works if you add an empty command section like in the example, but I still think that it should be possible to leave it out completely if I don't need it.

@obestwalter
Copy link
Member

@obestwalter obestwalter commented Jan 6, 2017

Ehh sorry - no it doesn't ... I had 2.4.1 installed from a test before. There it works with an empty command section. In 2.5.0 it throws an error, so we have a mismatch between documentation and behaviour now.

A regression we have here indeed.

@obestwalter
Copy link
Member

@obestwalter obestwalter commented Feb 12, 2017

The actual aim of this issue was achieved, but a bit overreached by throwing an error now, which we'll track here - so closing this one.

@obestwalter
Copy link
Member

@obestwalter obestwalter commented Mar 6, 2017

Reopening this as we had to revert this change, because instead of the suggested warning a missing/empty commands section caused an error, which is not what we want.

@obestwalter obestwalter reopened this Mar 6, 2017
@obestwalter
Copy link
Member

@obestwalter obestwalter commented Mar 6, 2017

Thinking about it, I am not even sure anymore if we should warn the user, because having no commands is a perfectly valid use case, which is even documented.

So if somebody wants to have another go at this, I would suggest we even dial it down to informing the user that there are no commands to be used, instead of warning them, when this might exactly be what they want.

Maybe it is not even worth the fuss, so we might as well close this a wontfix?

@vlcinsky
Copy link

@vlcinsky vlcinsky commented Mar 6, 2017

I would be perfectly happy with wontfix.
Informing the user instead of warning or error message is also good solution (I would prefer the wontfix).

@obestwalter
Copy link
Member

@obestwalter obestwalter commented Mar 6, 2017

@vlcinsky - thanks. Sometimes the best features are the ones that are not implemented :)

@obestwalter obestwalter closed this Mar 6, 2017
@vlcinsky
Copy link

@vlcinsky vlcinsky commented Mar 6, 2017

@obestwalter you made my day.

Btw: ability to remove already introduced feature requires real self-confidence. When ego(s) start(s) screaming "keep my footprint there, it's mine", only people who value usability over footprints have clear answer. (at the same time thanks @The-Compiler for bringing ideas - it counts and contributes too).

@The-Compiler
Copy link
Member

@The-Compiler The-Compiler commented Mar 6, 2017

I don't even remember why I opened this issue, so I'm fine with that 😆

@obestwalter
Copy link
Member

@obestwalter obestwalter commented Mar 6, 2017

Oh you opened this @The-Compiler - sorry for muddling this up - I am glad that the result is correct even if it was based on wrong assumptions :)

@tox-dev tox-dev locked and limited conversation to collaborators Jan 14, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

4 participants