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

Better warnings when configuration directives in wrong section #3188

Open
0cjs opened this issue Jan 17, 2024 · 6 comments
Open

Better warnings when configuration directives in wrong section #3188

0cjs opened this issue Jan 17, 2024 · 6 comments
Labels
help:wanted Issues that have been acknowledged, a solution determined and a PR might likely be accepted.

Comments

@0cjs
Copy link
Contributor

0cjs commented Jan 17, 2024

Issue

As a new user to tox browsing through the rather long Configuration documentation, it wasn't entirely clear to me when in the middle of the page which particular section a directive should go in. I set ignore_base_python_conflict = false in the [testenv] section, but this did not take effect because it should have gone in the [tox] section.

I discovered this only because, for other reasons, I happened to be examining the output of tox config and noticed some # !!! unused: ignore_base_python_conflict lines in it.

It would be nice if tox could emit a warning during test runs in situations like this.

Additional Documentation Suggestion

Further, it might clarify the documentation somewhat if the "Core" and "tox environment" sections made it a bit more clear that each is talking about a different section of the configuration file. For example, after these headings a note could be added along the lines of:

Core

The following options belong in the [tox] section of tox.ini or the [tox:tox] section of setup.cfg.

...

tox environment

The following options belong in the [testenv] section of tox.ini or setup.cfg.

Alternatively, and perhaps better, would be to divide the "Configuration" page into three: a main page containing the first top-level section and two sub-pages containing the "Core" and "tox environment" sections, respectively.

(I can probably figure out how to submit a patch for the former documentation change; I don't know how to do the latter.)

@gaborbernat
Copy link
Member

PR welcome 👍

@0cjs
Copy link
Contributor Author

0cjs commented Jan 19, 2024

PR welcome 👍

So, before I get into this, will you accept a PR that makes tox exit with an error when it encounters a configuration directive it doesn't understand anywhere in tox.ini?

@gaborbernat
Copy link
Member

I don't think that's possible to detect, or it would make tox a lot slower.

@0cjs
Copy link
Contributor Author

0cjs commented Jan 19, 2024

That sounds odd; ConfigSet.unused() seems to detect them: that's how tox config prints the # !!! unused: lines.

As for speed, on my quite slow mini-desktop (Celeron N5105 CPU):

  • tox xyzzy takes about .29 seconds to error out with unknown argument.
  • tox -e xyzzy takes about .29 seconds to say it can't find that environment.
  • tox - e xyzzy with [testenv:xyzzy] and command = true takes about .64 seconds.
  • tox -e py3.9-pytest7,py3.10-pytest7, which runs just two pytest tests in each environment, takes about 1.9 seconds.
  • tox config takes .46 seconds.

From this, my initial guess would be that this would add about 1/5 second run time on my machine, and much less on any modern laptop, not to say desktop. (By "modern" I mean pretty much anything from the last ten years; an Intel Core i7-3450 from 2012 is around 50% faster than this Celeron.)

So is my calculation off here, or is an tenth of a second or less on most tox runs an issue? What would you say is the average amount of time a tox run takes?

@gaborbernat
Copy link
Member

gaborbernat commented Jan 19, 2024

That sounds odd; ConfigSet.unused() seems to detect them: that's how tox config prints the # !!! unused: lines.

The reason ConfigSet.unused is a warning and not an error today is because doesn't mean that it's actually unused. For example:

[tox]
env_list =
    wheel
    sdist
requires = magic-tox-plugin
magic_plugin_config = yes

If this plugin is not installed, tox will provision an environment with that plugin. In this case, the host environment will make the magic_plugin_config unused, but is used by the provisioned environment.

Finally, I'm not against such a feature, but would need to be behind an opt-in flag. That is because many people might have unused configurations, and we can't suddenly break them (e.g., settings that were alive in tox 3, but meanwhile have been removed).

I think the documentation improvements are much more actionable/controversial so that would be an easier PR, when I suggested that PR welcome 😊

@0cjs
Copy link
Contributor Author

0cjs commented Jan 19, 2024

I think the documentation improvements are much more actionable/controversial so that would be an easier PR, when I suggested that PR welcome

Ah, ok. This is why it's good to discuss things with people, rather than just assume they understand everything you're thinking.

PR #3194 submitted. I had a quick go at splitting the config section into a top page and two sub-pages (one for "Core" and one for "tox environment"), but no luck with figuring out how to do that. The Sphinx toctree documentation doesn't seem to cover this, and simply creating a subdir with an index and a couple of other files in it doesn't seem to work.

@gaborbernat gaborbernat added the help:wanted Issues that have been acknowledged, a solution determined and a PR might likely be accepted. label Mar 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help:wanted Issues that have been acknowledged, a solution determined and a PR might likely be accepted.
Projects
None yet
Development

No branches or pull requests

2 participants