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

Replace len() Zero Checks with Pythonic Empty Sequence Checks #4298

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

andylizf
Copy link
Contributor

@andylizf andylizf commented Nov 8, 2024

Replace len() comparisons with 0/1 with pythonic empty sequence checks, following Google Python Style Guide's recommendation:

For sequences (strings, lists, tuples), use the fact that empty sequences are false, so if seq: and if not seq: are preferable to if len(seq): and if not len(seq): respectively.

Also fixed some typing annotations along the way.

Tested (run the relevant ones):

  • Code formatting: bash format.sh
  • Any manual or new tests for this PR (please specify below)
  • All smoke tests: pytest tests/test_smoke.py
  • Relevant individual smoke tests: pytest tests/test_smoke.py::test_fill_in_the_name
  • Backward compatibility tests: conda deactivate; bash -i tests/backward_compatibility_tests.sh

@andylizf
Copy link
Contributor Author

andylizf commented Nov 8, 2024

@cblmemo PTAL, thanks!

@Michaelvll
Copy link
Collaborator

@andylizf
Copy link
Contributor Author

@Michaelvll Precisely. As mentioned, this modification was just made based on the Google Python Style Guide. Please refer to: https://google.github.io/styleguide/pyguide.html#2144-decision.

@andylizf
Copy link
Contributor Author

@Michaelvll By the way, when you have a moment, could you take a look at #4250 ? Thanks a lot!

@Michaelvll
Copy link
Collaborator

Ahh, thanks for fixing these! I misread the message. Looks pretty good at the first glance. cc'ing @cblmemo to take a closer look. : )

Copy link
Collaborator

@cblmemo cblmemo left a comment

Choose a reason for hiding this comment

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

Thanks for refactoring this @andylizf ! Mostly looks good to me. Left some discussions.

Also, pls run smoke test to make sure it does not break anything ;) (reminder: Pls run the smoke tests on GCP. to do so, you can temporarily disable aws on your laptop.

format.sh Outdated Show resolved Hide resolved
sky/clouds/service_catalog/common.py Outdated Show resolved Hide resolved
sky/clouds/service_catalog/common.py Outdated Show resolved Hide resolved
sky/resources.py Show resolved Hide resolved
sky/serve/serve_utils.py Outdated Show resolved Hide resolved
sky/serve/serve_utils.py Show resolved Hide resolved
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