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

Require isort on all Python files. #8420

Merged
merged 5 commits into from
Nov 8, 2022
Merged

Conversation

trivialfis
Copy link
Member

After initial refactoring of tests, the required amount of changes for running isort is significantly smaller. Let's require it on the CI from now on.

All changes in this PR except for the linter script are made by isort.

@trivialfis trivialfis requested a review from hcho3 November 4, 2022 15:15
@hcho3
Copy link
Collaborator

hcho3 commented Nov 4, 2022

Doesn't black run isort internally?

@trivialfis
Copy link
Member Author

No, they are different linters.

@trivialfis
Copy link
Member Author

isort --profile=black ..

But we have this option to make them work together.

@trivialfis
Copy link
Member Author

trivialfis commented Nov 4, 2022

Just a brief note on why are we doing all these refactorings.

  • Init estimation for regression. #8272 requires changes to PySpark tests since many of them are written with hardcoded expected results.
  • But pyspark and dask tests are conflicting with each other for unknown reasons in new PRs so we split them up into test_distributed.
  • Then PySpark tests use the python unittest module, which conflicts with pytest utilities like skipif and fixtures. As a result, we need to clean up the PySpark tests to transition to pytest.

Lastly, having linters and a type checker can help us better maintain these growing Python scripts. This PR makes isort a requirement, but black is still optional for most of the scripts.

@trivialfis trivialfis merged commit 0d3da98 into dmlc:master Nov 8, 2022
@trivialfis trivialfis deleted the isort branch November 8, 2022 04:59
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.

2 participants