tox-dev / tox

Command line driven CI frontend and development task automation tool.
https://tox.wiki
MIT License
3.68k stars 521 forks source link

Better warnings when configuration directives in wrong section #3188

Open 0cjs opened 9 months ago

0cjs commented 9 months ago

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 commented 9 months ago

PR welcome 👍

0cjs commented 9 months ago

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 commented 9 months ago

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

0cjs commented 9 months ago

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):

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 commented 9 months ago

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 commented 9 months ago

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.