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

style(api): Automatically sort imports #15171

Closed
wants to merge 5 commits into from
Closed

style(api): Automatically sort imports #15171

wants to merge 5 commits into from

Conversation

SyntaxColoring
Copy link
Contributor

@SyntaxColoring SyntaxColoring commented May 13, 2024

Overview

The ordering of import statements is a boring style concern, so let's automate it like we automate formatting with Black.

Among other things, this is helpful for refactors like #15162, where we want to add a new import to ~60 files.

This goes towards EXEC-456.

To avoid horrific merge conflicts, we should wait to merge this until we merge v7.3.0 back into edge.

Test plan

None needed.

Changelog

Via isort:

  • During make -C api lint, check that imports are sorted.
  • During make -C api format, automatically sort imports.

Review requests

Review commit-by-commit to separate my manual changes from the automated changes from the initial run of isort.

  • Any reason we shouldn't do this?
  • Any reason to adjust isort's configuration from the default?
  • Any files that need special manual attention?

If we like this, we can do the same thing in our other Python projects.

Risk assessment

Low.

Copy link
Member

@sfoster1 sfoster1 left a comment

Choose a reason for hiding this comment

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

I think this is a good idea and I think that actually we don't really need to wait to merge it; 7.3.0 has gotten some regular mergebacks (and could use another) and only changes to imports should conflict - there's not that many of them.

@sfoster1
Copy link
Member

That said, looking at the failed tests we're gonna have to do some followups lol

@SyntaxColoring
Copy link
Contributor Author

SyntaxColoring commented May 20, 2024

I think this is a good idea and I think that actually we don't really need to wait to merge it; 7.3.0 has gotten some regular mergebacks (and could use another) and only changes to imports should conflict - there's not that many of them.

It looks like import reordering can bring up latent circular dependency problems. I'll hold off on this until after the final mergeback, on the off chance that it happens there. We don't want whoever does the mergeback to have to deal with that kind of thing.

@SyntaxColoring
Copy link
Contributor Author

Closing this for now. It'll probably be a little bit before I get to it.

At the time of writing, we "just" need to resolve the circular dependency problems described in #15234 to unblock this.

I also suspect robot-server and shared-data will go way smoother, so maybe we'll do those first.

@SyntaxColoring SyntaxColoring deleted the isort_api branch May 21, 2024 22:02
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