[1] Add tox generation script, but don't use it yet #3971
Conversation
❌ 1 Tests Failed:
View the top 1 failed tests by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
sentrivana
requested review from
antonpirker,
sl0thentr0py and
szokeasaurusrex
| "*": ["Werkzeug"], | ||
| "<3.0": ["Werkzeug<2.1.0"], |
There was a problem hiding this comment.
Does order matter here?
There was a problem hiding this comment.
No, any order is fine
scripts/populate_tox/README.md
Outdated
| ``` | ||
| "aiohttp": { | ||
| ... | ||
| "python": ">=3.7", | ||
| } | ||
| ``` |
There was a problem hiding this comment.
It is strange that the snippets before and after this one use deps, but this one does not. Is this intentional? If yes, maybe reordering would make sense
There was a problem hiding this comment.
So this was an example for the paragraph talking about the python key, specifically. I can reorganize the README a bit so that it's clearer what belongs together. I'm thinking subheadings for the different keys would already help a lot.
There was a problem hiding this comment.
Reorganized a bit in 474c679
scripts/populate_tox/README.md
Outdated
| The following can be set as a rule: | ||
| - `*`: packages will be always installed | ||
| - a version specifier on the main package (e.g. `<=0.32`): packages will only | ||
| be installed if the main package falls into the version bounds specified | ||
| - specific Python version(s) in the form `py3.8,py3.9`: packages will only be | ||
| installed if the Python version matches one from the list |
There was a problem hiding this comment.
I find this description somewhat unclear.
The first line I would change to something like "The rule must be set to one of the following." Compared to the current first line, which is not specific regarding whether there might be some other valid values for the rule, my proposal clarifies that these are the only allowed values.
Second, I would explicitly clarify that the packages that are installed are always the ones listed under that rule.
| CLASSIFIER_PREFIX = "Programming Language :: Python :: " | ||
|
|
||
|
|
||
| IGNORE = { |
There was a problem hiding this comment.
Are all integrations listed in here?
There was a problem hiding this comment.
In this PR, yes, effectively making the script almost no-op. It still generates the tox.ini file, but without any auto-generated integration entries. This then happens in the other PRs.
scripts/populate_tox/populate_tox.py
Outdated
| return hasher.hexdigest() | ||
|
|
||
|
|
||
| def main(fail_on_changes: bool = False) -> None: |
There was a problem hiding this comment.
This function seems a bit long – I think we should try to break it up into smaller functions. At a minimum, probably the code in the loop could be its own function
There was a problem hiding this comment.
Broken up in c96bf0c
scripts/populate_tox/populate_tox.py
Outdated
|
|
||
| if __name__ == "__main__": | ||
| fail_on_changes = len(sys.argv) == 2 and sys.argv[1] == "--fail-on-changes" | ||
| main(fail_on_changes) |
There was a problem hiding this comment.
I think it might be a good idea to add some tests for this script. What do you think?
Add:
In this PR, the script is set to ignore all integrations, so no tox configuration is actually added. However, it's still the script actually generating the real
tox.inifrom thetox.jinjatemplate.See follow-up PRs for more.
Supersedes #3920