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.

Sign up for GitHub

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

Jump to bottom

Sort import statements within project #777

Merged
merged 25 commits into from
Jan 21, 2022
Merged

Sort import statements within project #777

merged 25 commits into from
Jan 21, 2022

Conversation

cidrblock
Copy link
Collaborator

@cidrblock cidrblock commented Jan 20, 2022 •
edited
Loading

  • Adds isort configuration to pyproject.toml (a default location for isort configuration)
  • Removes all I00* exemptions from .flake8
  • Removes the all file exclusions from precommit (it remains a vetting/manual check)
  • Updates import statements across all files as found using isort via tox -e lint-vetting

After several iterations this configuration for isort was found to most closely align with the current sort order and minimize the number of changes.

Note:

  • Future PRs issues can/may/should/could be raised to revisit the isort configuration
  • A future PR will be put in to move the pre-commit config out of tox lint-vetting to tox lint and modify the contributors guide
  • See the pyproject.toml file for an explanation of each setting
  • The reverse_relative settings will be remove in a follow-up PR, issue Remove reverse_relative from pyproject.toml (isort) after initial PR. #780

Closes #715

@cidrblock cidrblock self-assigned this Jan 20, 2022
@cidrblock cidrblock requested review from a team, relrod, ssbarnea and webknjaz January 20, 2022 12:07
webknjaz
webknjaz reviewed Jan 20, 2022
View reviewed changes
.pre-commit-config.yaml Outdated Show resolved Hide resolved
webknjaz
webknjaz reviewed Jan 20, 2022
View reviewed changes
.pre-commit-config.yaml Outdated Show resolved Hide resolved
webknjaz
webknjaz reviewed Jan 20, 2022
View reviewed changes
.pre-commit-config.yaml Outdated Show resolved Hide resolved
webknjaz
webknjaz reviewed Jan 20, 2022
View reviewed changes
.pre-commit-config.yaml Outdated Show resolved Hide resolved
webknjaz
webknjaz reviewed Jan 20, 2022
View reviewed changes
docs/conf.py Outdated Show resolved Hide resolved
webknjaz
webknjaz reviewed Jan 20, 2022
View reviewed changes
.pre-commit-config.yaml Outdated Show resolved Hide resolved
webknjaz
webknjaz reviewed Jan 20, 2022
View reviewed changes
pyproject.toml Outdated Show resolved Hide resolved
webknjaz
webknjaz reviewed Jan 20, 2022
View reviewed changes
pyproject.toml Outdated Show resolved Hide resolved
webknjaz
webknjaz reviewed Jan 20, 2022
View reviewed changes
pyproject.toml Outdated Show resolved Hide resolved
webknjaz
webknjaz suggested changes Jan 20, 2022
View reviewed changes
Copy link
Member

@webknjaz webknjaz left a comment

Choose a reason for hiding this comment

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

Per slack discussions, let's implement #778 in a standalone prerequisite PR so that there is at least a basic sanity check for whether this big patch introduces any obvious import loops.

@cidrblock cidrblock mentioned this pull request Jan 20, 2022
Merged
ansible-zuul bot pushed a commit that referenced this pull request Jan 20, 2022
@cidrblock
Test for circular imports in project (#779)
cd27e3c 
Test for circular imports in project

Resolves: #778
Test for circular imports in all local packages and modules.
This ensures all internal packages can be imported right away without any need to import some other module before doing so.
Related: #777

Reviewed-by: Sviatoslav Sydorenko <webknjaz+github/profile@redhat.com>
Reviewed-by: None <None>
Reviewed-by: Sorin Sbarnea <sorin.sbarnea@gmail.com>
Reviewed-by: Bradley A. Thornton <bthornto@redhat.com>
@cidrblock cidrblock mentioned this pull request Jan 20, 2022
Closed
@cidrblock cidrblock requested a review from webknjaz January 21, 2022 12:00
@webknjaz webknjaz mentioned this pull request Jan 21, 2022
Closed
This was referenced Jan 21, 2022
Closed
Closed
webknjaz
webknjaz approved these changes Jan 21, 2022
View reviewed changes
Copy link
Member

@webknjaz webknjaz left a comment

Choose a reason for hiding this comment

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

I've filed #784 and #785 for follow-ups so this should be good to go now.

@webknjaz webknjaz added the gate label Jan 21, 2022
ansible-zuul[bot]
ansible-zuul bot approved these changes Jan 21, 2022
View reviewed changes
Copy link
Contributor

@ansible-zuul ansible-zuul bot left a comment

Choose a reason for hiding this comment

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

LGTM!

@ansible-zuul ansible-zuul bot merged commit c3b237b into ansible:main Jan 21, 2022
@cidrblock cidrblock mentioned this pull request Jan 21, 2022
Merged
ansible-zuul bot pushed a commit that referenced this pull request Jan 24, 2022
@cidrblock
import order (#786)
90fa5db 
Change the order of relative imports

This modifies the order of relative import statements.
Fixes: #780
Prior to this change the order was
import .sibling
import ..parent
import ...grandparent

after this change
import ...grandparent
import ..parent
import .sibling

The resulting order is the default for isort which decreases the amount of custom configuration within the repo and results in a sorting order starting with "least local" to "most local".
This follow on work after #777, which was intended to minimize the changes in the repo related to the cleanup. #779 was also implemented to help detect any issues arising from a change in order.
Some conversation and history about the ordering can be found here: PyCQA/isort#417

Reviewed-by: Sviatoslav Sydorenko <webknjaz+github/profile@redhat.com>
Reviewed-by: None <None>
cidrblock added a commit to cidrblock/ansible-navigator that referenced this pull request Jan 24, 2022
@cidrblock
import order (ansible#786)
3361835 
Change the order of relative imports

This modifies the order of relative import statements.
Fixes: ansible#780
Prior to this change the order was
import .sibling
import ..parent
import ...grandparent

after this change
import ...grandparent
import ..parent
import .sibling

The resulting order is the default for isort which decreases the amount of custom configuration within the repo and results in a sorting order starting with "least local" to "most local".
This follow on work after ansible#777, which was intended to minimize the changes in the repo related to the cleanup. ansible#779 was also implemented to help detect any issues arising from a change in order.
Some conversation and history about the ordering can be found here: PyCQA/isort#417

Reviewed-by: Sviatoslav Sydorenko <webknjaz+github/profile@redhat.com>
Reviewed-by: None <None>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ansible-zuul ansible-zuul[bot] ansible-zuul[bot] approved these changes

@webknjaz webknjaz webknjaz approved these changes

@relrod relrod Awaiting requested review from relrod relrod is a code owner automatically assigned from ansible/devtools

@ssbarnea ssbarnea Awaiting requested review from ssbarnea ssbarnea is a code owner automatically assigned from ansible/devtools

Assignees

@cidrblock cidrblock

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@cidrblock @ssbarnea @webknjaz