-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
10248 configure isort pre-commit hook #1655
Conversation
pre-commit.ci autofix |
for more information, see https://pre-commit.ci
Thanks for working on this. I think that this is much needed change for the project. The changes looks good, with the exception of the newly created module. It would have been nice to have a quick description of this error and the solution in the PR description. I am not sure I fully understand to main cause that trigger the generation of a new module. I am a bit worries that this import error was only detected by accident via pydoctor. Can we also use the same import method as pydoctor in one of twisted own tests to make sure we cover this import path? To keep the size of this PR low and avoid further changes, can this PR be merged with an I am a bit worried that this change might trigger side-effects with project using Twisted. Also, I think that if regressions are found due to import order, it's important to have available a quick fix option so that we don't have always do a code refactoring. Now, refactoring the code so that the import order is not that import is a good thing. But, with this new refactoring I don't really see the purpose of twisted.web.template. After this lands, I think that it would help to send an email to the mailing list to let twisted users know that we made some changes to the import order and they should run a check of the new code now. Maybe even trigger a 21.8.0 release, with a pre-release on pypi. I also tried to read the info from #1132 I don't know if Maybe we can stop building
From the error of the I have done a few more (7 times while reviewing the module rename) runs |
the error occurs for me with |
This reverts commit b9445eb.
I've tried another re-run and without the tox depends change locally and it does indeed pass every time |
the pyproject.toml lists an allowlist of un-isorted modules. But really it's better to merge modules with a cyclic dependency |
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.
All good. Thanks.
I have only reviewed the manual changes... and not the automated changes made by isort.
Since this PR has no description, I assume that the purpose for using isort is to get consistent code, similar to black
.
I don't have a better name for the new private module.
Since this is private module we can change it at any time.
Also, this PR is only moving code to a private module without changing the public API.
Even if things are not perfect now, since this is private we can update it later without generating any extra breakage.
The coverage is not 100% but that should be ok for this PR.
Thanks again @graingert for the hard work!
One minor comment. Since the import order was changed, 3rd party users, like pydoctor, might end up with import errors. This is why I plan to do a 21.8.0rc1 release soon to get some more feedback about this change. Thanks! |
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.
All good. Thanks.
Scope and purpose
Contributor Checklist:
tox -e lint
to format my patch to meet the Twisted Coding Standard#
character).review
to the keywords field in Trac, and putting a link to this PR in the comment; it shows up in https://twisted.reviews/ now.The first line is automatically generated by GitHub based on PR ID and branch name.
The other lines generated by GitHub should be replaced.