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

10248 configure isort pre-commit hook #1655

Merged
merged 15 commits into from
Aug 8, 2021
Merged

Conversation

graingert
Copy link
Member

@graingert graingert commented Aug 4, 2021

Scope and purpose

Contributor Checklist:

  • The associated ticket in Trac is here: https://twistedmatrix.com/trac/ticket/10248
  • I ran tox -e lint to format my patch to meet the Twisted Coding Standard
  • I have created a newsfragment in src/twisted/newsfragments/ (see: News files)
  • The title of the PR starts with the associated Trac ticket number (without the # character).
  • I have updated the automated tests and checked that all checks for the PR are green.
  • I have submitted the associated Trac ticket for review by adding the word 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 merge commit will use the below format
    The first line is automatically generated by GitHub based on PR ID and branch name.
    The other lines generated by GitHub should be replaced.
Merge pull request #123 from twisted/4356-branch-name-with-trac-id

Author: <github_username>, <github_usernames_if_more_authors>
Reviewer: <github_username>, <github_username_if_more_reviewers>
Fixes: ticket:<trac_ticket_number>, ticket:<another_if_more_in_one_PR>

Long description providing a summary of these changes.
(as long as you wish)

@graingert graingert changed the title configure isort 10248 configure isort pre-commit hook Aug 4, 2021
@graingert
Copy link
Member Author

pre-commit.ci autofix

@graingert graingert requested a review from wsanchez August 6, 2021 07:37
@graingert graingert closed this Aug 7, 2021
@graingert graingert reopened this Aug 7, 2021
@graingert graingert marked this pull request as ready for review August 8, 2021 06:28
@graingert graingert requested a review from a team August 8, 2021 06:39
@adiroiban
Copy link
Member

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 # isort: skip so that we don't need to create a new module?

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.
I now see it as a public gatekeeper to some private API from twisted_utils.py ... maybe _template_util.py should be just be named _template.py


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 saw that in the end the PR was merged with isort configuration...but isort was not really used in the project.


I don't know if narrativedocs depends on apidocs.
narrativedocs will trigger an internal apidocs build, as the API documentation is now integrated into narrative documentation... to make it work with Read The Docs.

Maybe we can stop building tox -e apidocs in CI.
I left the apidocs tox env for the case in which you want a quicker debugging / testing of the API docs content.

$ tox -p2 -e narrativedocs,apidocs works for me on trunk.

From the error of the apidocs build (https://github.com/twisted/twisted/runs/3260163989#step:5:80) I don't see any hint that the error is caused by narrativedocs

I have done a few more (7 times while reviewing the module rename) runs $ tox -p2 -e narrativedocs,apidocs runs on my laptop and all looks ok.

@graingert
Copy link
Member Author

I have done a few more (7 times while reviewing the module rename) runs $ tox -p2 -e narrativedocs,apidocs runs on my laptop and all looks ok.

the error occurs for me with tox -p2 -re narrativedocs,apidocs

@graingert graingert requested a review from adiroiban August 8, 2021 11:47
@graingert
Copy link
Member Author

I've tried another re-run and without the tox depends change locally and it does indeed pass every time

@graingert
Copy link
Member Author

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.

the pyproject.toml lists an allowlist of un-isorted modules. But really it's better to merge modules with a cyclic dependency

Copy link
Member

@adiroiban adiroiban left a 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!

@adiroiban
Copy link
Member

One minor comment.
It would help to have a release note for this change.

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!

@graingert graingert requested a review from adiroiban August 8, 2021 13:37
Copy link
Member

@adiroiban adiroiban left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All good. Thanks.

@graingert graingert merged commit 1d439dd into twisted:trunk Aug 8, 2021
@graingert graingert deleted the isort branch August 8, 2021 17:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants