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

Project linter set to ruff #303

Merged
merged 8 commits into from
Apr 7, 2023

Conversation

RoshiniFernando
Copy link
Contributor

Situation 😊

Method I followed

  • I installed ruff version 0.0.255 using following command.
pip install ruff
  • Added a pyproject.toml to configure ruff and used the default configuration they have mentioned in the description here.
    Link to ruff Github Repo
  • Changed ci3.8.yml, ci3.9.yml, CONTRIBUTING.md and setup.py from flake8 to ruff.

Evaluation

  • I run following commands seperately.
ruff check /workspace/weather-tools/weather_dl
ruff check /workspace/weather-tools/weather_mv
ruff check /workspace/weather-tools/weather_sp
  • I got only E501 Line too long errors for all three.
  • So I changed a colon in line 52 /workspace/weather-tools/weather_dl/download_pipeline/clients.py file and checked about another error that can ruff can show us like this.

Screenshot 2023-03-15 at 10 27 47

My final opinion

  • I think the linter ruff works fine for this weather tools project now as my knowledge. @alxmrs I hope you will check and lead me as you're doing from the beginning to do any changes with this PR.
    Thank you so much !! 😊

@alxmrs
Copy link
Collaborator

alxmrs commented Mar 15, 2023

Hey Roshini! Thanks so much for the contributions.

I think I see why this hit so many lint errors: We use a max-line-length of 120. I think we'd like to keep this convention, as to not require too many file changes.

On that note: I think that this CL should address all link errors that ruff finds in this project thereafter.

@RoshiniFernando
Copy link
Contributor Author

  • I set line-length on pyproject.toml to 120.

  • Run ruff check to three main directories.

Screenshot 2023-03-16 at 07 34 12

@alxmrs
Copy link
Collaborator

alxmrs commented Mar 29, 2023

I'm happy to approve this PR once the CI passes. Can you take a look and see what the issue is?

@alxmrs
Copy link
Collaborator

alxmrs commented Apr 3, 2023

Hey Roshini, it looks like conda is having trouble installing ruff. PTAL:
Screenshot 2023-04-03 at 2 13 49 PM

@RoshiniFernando
Copy link
Contributor Author

Maybe it because of the ruff version. I'll check ❤️

@alxmrs
Copy link
Collaborator

alxmrs commented Apr 5, 2023

Hey, this is interesting -- I'm not sure exactly why, but it seems like conda doesn't like this ruff package version either.

https://github.com/google/weather-tools/actions/runs/4621234525/jobs/8173447586?pr=303

Can you try reproducing this locally? I will see if I can reproduce the issue a bit later on.

@RoshiniFernando
Copy link
Contributor Author

RoshiniFernando commented Apr 6, 2023

Thanks for your suggestions. I have reproduced the issue locally. Please find build logs on here

Copy link
Collaborator

@alxmrs alxmrs left a comment

Choose a reason for hiding this comment

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

LGTM :)

@alxmrs
Copy link
Collaborator

alxmrs commented Apr 6, 2023

Feel free to squash & merge when you're ready.

@RoshiniFernando
Copy link
Contributor Author

Can I know how to do the squash & merge? Seems like I'm not having writing access
to this repo @alxmrs 😊 I'm not seeing a merge button.

@alxmrs alxmrs merged commit caa9ba6 into google:main Apr 7, 2023
@alxmrs
Copy link
Collaborator

alxmrs commented Apr 7, 2023

Sorry about that! I didn't realize.

@RoshiniFernando
Copy link
Contributor Author

Thank you so much @alxmrs for the immense support through out this PR journey. I learnt a lot from here. Hope this project will do great in future ❤️😊

@RoshiniFernando RoshiniFernando deleted the Change_linter_to_ruff branch April 7, 2023 17:13
@alxmrs
Copy link
Collaborator

alxmrs commented Apr 11, 2023

Thanks for your contribution, and you're always welcome here :)

@alxmrs
Copy link
Collaborator

alxmrs commented Apr 12, 2023

Just used the new linter -- man, is it fast!! I'm really happy to have this change. Thank you :)

@RoshiniFernando
Copy link
Contributor Author

Yeah that's fast! I'm so glad to hear that @alxmrs 😊 Thank you to you too ❤️

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