-
Notifications
You must be signed in to change notification settings - Fork 589
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
Adding isort #2375
Adding isort #2375
Changes from all commits
8519476
a68dff3
6bac869
4c7ff4a
d715039
cb99b0c
2ae3daf
330e87d
d9a5f06
0147027
a6f577a
d16648b
fbe2f8b
cd0e0c4
780ec2a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -54,6 +54,14 @@ YAPF_EXCLUDES=( | |
'--exclude' 'sky/skylet/providers/ibm/**' | ||
) | ||
|
||
ISORT_YAPF_EXCLUDES=( | ||
'--sg' 'build/**' | ||
'--sg' 'sky/skylet/providers/aws/**' | ||
'--sg' 'sky/skylet/providers/gcp/**' | ||
'--sg' 'sky/skylet/providers/azure/**' | ||
Comment on lines
+59
to
+61
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (See other comment) Some files under these dirs seem formatted. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I added isort with the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Discussed with @Michaelvll. The imports being formatted may still hinder humans (us :)) looking at diffs when we're upgrading Ray. Can we skip formatting sky/skylet/providers/{aws,gcp,azure} even with the black profile, both in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes will do |
||
'--sg' 'sky/skylet/providers/ibm/**' | ||
) | ||
|
||
BLACK_INCLUDES=( | ||
'sky/skylet/providers/aws' | ||
'sky/skylet/providers/gcp' | ||
|
@@ -86,9 +94,12 @@ format_changed() { | |
|
||
# Format all files | ||
format_all() { | ||
yapf --in-place "${YAPF_FLAGS[@]}" "${YAPF_EXCLUDES[@]}" sky tests examples | ||
yapf --in-place "${YAPF_FLAGS[@]}" "${YAPF_EXCLUDES[@]}" sky tests examples llm | ||
} | ||
|
||
echo 'SkyPilot Black:' | ||
black "${BLACK_INCLUDES[@]}" | ||
|
||
## This flag formats individual files. --files *must* be the first command line | ||
## arg to use this option. | ||
if [[ "$1" == '--files' ]]; then | ||
|
@@ -102,8 +113,12 @@ else | |
format_changed | ||
fi | ||
echo 'SkyPilot yapf: Done' | ||
echo 'SkyPilot Black:' | ||
black "${BLACK_INCLUDES[@]}" | ||
|
||
echo 'SkyPilot isort:' | ||
isort sky tests examples llm docs "${ISORT_YAPF_EXCLUDES[@]}" | ||
|
||
isort --profile black -l 88 -m 3 "sky/skylet/providers/ibm" | ||
|
||
|
||
# Run mypy | ||
# TODO(zhwu): When more of the codebase is typed properly, the mypy flags | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 that we want to skip formatting sky/skylet/providers/{aws,gcp,azure}. Reason is those files are forked from upstream Ray, and each time when we do a Ray dependency upgrade, we do a diff against the upstream files to understand whether our genuine changes are affected.
However, it seems like this PR has changed files under those dirs?
.../ibm should be okay to be isort-ed since it's not forked from Ray.