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

Add support of isort>5 #16

Merged
merged 4 commits into from
Jul 5, 2020
Merged

Add support of isort>5 #16

merged 4 commits into from
Jul 5, 2020

Conversation

Mystic-Mirage
Copy link
Collaborator

@Mystic-Mirage Mystic-Mirage commented Jul 4, 2020

@Mystic-Mirage Mystic-Mirage requested a review from akaihola July 4, 2020 17:20
@akaihola
Copy link
Owner

akaihola commented Jul 4, 2020

Oops, I have a competing PR in #15 😄

@Mystic-Mirage
Copy link
Collaborator Author

Oops, I have a competing PR in #15

I'd prefer to add support of isort>5 first and then reimplement #13

@Mystic-Mirage
Copy link
Collaborator Author

@akaihola New isort makes config processing much easier for us.

@akaihola
Copy link
Owner

akaihola commented Jul 4, 2020

I think I managed to reduce unit test hassle a bit by using isort.api.sort_code_string() instead of isort.check_code() and isort.code() in #15.

Edit: Ah, isort.check_code() is an alias for isort.api.check_code_string(), and isort.code() aliases isort.api.sort_code_string(). Good.

Copy link
Owner

@akaihola akaihola left a comment

Choose a reason for hiding this comment

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

I'd prefer to add support of isort>5 first and then reimplement #13

Ok, yes, let's use your PR, that's probably cleaner.

I had to solve a couple of unit test challenges in #15 – maybe looking at those will help you then with #13?

result = SortImports(
file_contents=content,
check=True,
isort.check_code(code=content)
Copy link
Owner

Choose a reason for hiding this comment

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

Why is this call needed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This replaces check=True and writes error messages to stdout. See changes in test_main.

Copy link
Owner

Choose a reason for hiding this comment

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

Do we need the error messages?

And if we do, is there a cleaner way to capture them than stdout?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Do we need the error messages?

I don't know... Some tests depend on error messages. Probably, we could raise an exception if check_code is False

And if we do, is there a cleaner way to capture them than stdout?

Hm... isort has hardcoded prints to write to stdout

Copy link
Owner

Choose a reason for hiding this comment

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

Well, I had to include those error messages in the test assertions, because in isort<5 there was no way to get rid of the messages.

Now that we can run isort without polluting stdout with error messages, I think we should do that for simplicity's sake, and fix the tests as well – unless we get some benefit from having those error messages.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I thought check=False in isort<5 was for this?
I vote to get rid of error messages in another PR in order to keep this PR clear.

Copy link
Owner

Choose a reason for hiding this comment

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

I vote to get rid of error messages in another PR in order to keep this PR clear.

I created another PR: #17

@akaihola akaihola mentioned this pull request Jul 4, 2020
3 tasks
@akaihola
Copy link
Owner

akaihola commented Jul 4, 2020

We'll need this in setup.cfg:

[options.extras_require]
isort =
    isort>=5.0.1

(There seems to be a bug in isort 5.0.0)

@Mystic-Mirage
Copy link
Collaborator Author

I had to solve a couple of unit test challenges in #15 – maybe looking at those will help you then with #13?

All tests pass after commenting --isort

@Mystic-Mirage Mystic-Mirage requested a review from akaihola July 4, 2020 18:02
@akaihola akaihola merged commit 7b1862e into akaihola:master Jul 5, 2020
@Mystic-Mirage Mystic-Mirage deleted the isort_v5 branch July 10, 2020 12:59
@akaihola akaihola added this to the 1.0.0 milestone Jul 11, 2020
@akaihola akaihola added the bug Something isn't working label Jul 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants