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

Sort imports with Isort. #102

Merged
merged 5 commits into from
Jun 23, 2022
Merged

Sort imports with Isort. #102

merged 5 commits into from
Jun 23, 2022

Conversation

TheQuinbox
Copy link
Contributor

@TheQuinbox TheQuinbox commented Feb 16, 2022

Link to issue number:

None

Summary of the issue:

In Pep-8, it's convention to sort imports a certain way. There's a tool (Isort) that does this for us.

Description of how this pull request fixes the issue:

Added Isort to requirements-dev, and ran it on the entire codebase.

Testing performed:

None necessary (Isort doesn't change functionality), similar to Black.

Known issues with pull request:

None

@mush42
Copy link
Collaborator

mush42 commented Feb 16, 2022

Hi @TheQuinbox

I tried isort last week and after running it on the source code bookworm failed to run.

We've some problems with circular imports, unless we fix those, the tool will, actually, effect the functionality.

I'll be keeping an eye on this PR.

Best

@TheQuinbox
Copy link
Contributor Author

@mush42, I don't actually see how that's possible. AFAIK, Isort just moves code around (puts line breaks, indents extra long lines, etc). None of that should cause circular imports.

I would test, but I'm currently unable to run Bookworm from source for... some reason. What files were causing the problem?

@pauliyobo
Copy link
Collaborator

Hello.
@TheQuinbox I was able to solve the circular import issues, or at least solve enough to make bookworm run in a test branch.
How would you like to progress on this?

@TheQuinbox
Copy link
Contributor Author

@pauliyobo, if you could make it run, resolve the merge conflicts and merge it, that would be amazing! :). If not, I can close this, no issue there.

@pauliyobo pauliyobo merged commit 812fb22 into blindpandas:develop Jun 23, 2022
@mush42
Copy link
Collaborator

mush42 commented Jun 25, 2022

@pauliyobo Is it safe to re-run isort on the code base without reservations?

@pauliyobo
Copy link
Collaborator

pauliyobo commented Jun 26, 2022

@mush42 It should be yes.
Although I am personally thinking that we should probably automate this process with something like pre-commit in order to maintain consistency. Would you have any problem with this? I am happy to open an issue if not.

@mush42
Copy link
Collaborator

mush42 commented Jun 26, 2022

No. I have no problems with adding pre-commit.
Do you have time to add this? Or should I add it myself?

@pauliyobo
Copy link
Collaborator

I do. I'll add this tomorrow.

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.

3 participants