-
Notifications
You must be signed in to change notification settings - Fork 522
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
[1] Add tox generation script, but don't use it yet #3971
Conversation
❌ 52 Tests Failed:
View the top 3 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for splitting up the PR, I think it is much easier to review now!
I added some questions and suggestions
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it might be a good idea to add some tests for this script. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks better, but I am still struggling to understand some parts of the code. I think more comments and some small refactors would help.
Please see the individual comments; we could also discuss offline if you think that would help
Co-authored-by: Daniel Szoke <[email protected]>
Co-authored-by: Daniel Szoke <[email protected]>
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.ini
from thetox.jinja
template.See follow-up PRs for more.
Supersedes #3920