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

[refactor] Integrate Ruff for lint & format #3125

Merged
merged 11 commits into from
Jul 10, 2024

Conversation

Sharathmk99
Copy link
Contributor

@Sharathmk99 Sharathmk99 commented Mar 19, 2024

Ruff integration for lint & format to maintain code standardisation.

Fixes - #3124 #3099

It works perfectly fine with vscode extension

@Sharathmk99
Copy link
Contributor Author

@alberttorosyan @mihran113 would appreciate review comments on this PR. This PR changes only import orders, double quotes, simple limiting rules.

@Sharathmk99 Sharathmk99 changed the title Ruff integration for lint & format [feat] Ruff integration for lint & format Mar 20, 2024
@mihran113 mihran113 changed the title [feat] Ruff integration for lint & format [refactor] Integrate Ruff for lint & format Mar 20, 2024
@Sharathmk99
Copy link
Contributor Author

@mihran113 Can I get review of the PR? Please let me know if I need to change anything. Thanks

@Sharathmk99
Copy link
Contributor Author

Hi All,
Is it possible to review the PR? I’m open to make changes if required. Please let us know if this PR makes sense. Thanks.

@alberttorosyan
Copy link
Member

Hi @Sharathmk99! Sorry for late response; we are still in the process of deciding on the linting and formatting, but will most probably end up using combination of black and flake8.
Would you mind closing this PR for now? We can definitely reopen this conversation once there's a final decision on the tools to be used.

@Sharathmk99
Copy link
Contributor Author

Hi @Sharathmk99! Sorry for late response; we are still in the process of deciding on the linting and formatting, but will most probably end up using combination of black and flake8.
Would you mind closing this PR for now? We can definitely reopen this conversation once there's a final decision on the tools to be used.

Thank you for your response.
Sure I can close the PR, but personally I think ruff can replace both black and flake8 and it's super fast on aim code base. If we are going with black and flake8 can we start work on that as it's important for us internally as well.

@Sharathmk99 Sharathmk99 reopened this Jun 29, 2024
@Sharathmk99
Copy link
Contributor Author

Sharathmk99 commented Jun 29, 2024

@SGevorg @alberttorosyan As agreed I have reopened the PR and configured to use single quotes.
Please do review and let me know if any questions. Thanks

Copy link
Member

@alberttorosyan alberttorosyan left a comment

Choose a reason for hiding this comment

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

@Sharathmk99, sorry for late response; took some time to go through the changes.
Everything looks good, seems there are some minor fixes left (the workflow fails on the code style checks due to some import redefinitions). Once done, we can proceed with merge

@Sharathmk99
Copy link
Contributor Author

@Sharathmk99, sorry for late response; took some time to go through the changes.
Everything looks good, seems there are some minor fixes left (the workflow fails on the code style checks due to some import redefinitions). Once done, we can proceed with merge

@alberttorosyan Thank you for the review. I have made the changes and tested locally. Could you please approve CI workflow so I can test in Github actions as well?

Thank you!!

@mihran113
Copy link
Contributor

Hey @Sharathmk99! Seems workflow passes the steps required(performance tests are not up to date, so that fail is expected). @alberttorosyan is away for the upcoming week, so if there're no more changes pending from your end, I'll proceed to merge the PR.

@mihran113
Copy link
Contributor

Hm, after I've fixed the merge conflict, the new check shows that one more file needs reformatting (not the conflicting file). @Sharathmk99 could you please update that as well?
https://github.com/aimhubio/aim/actions/runs/9873153670/job/27264800270?pr=3125

@Sharathmk99
Copy link
Contributor Author

Hm, after I've fixed the merge conflict, the new check shows that one more file needs reformatting (not the conflicting file). @Sharathmk99 could you please update that as well? https://github.com/aimhubio/aim/actions/runs/9873153670/job/27264800270?pr=3125

On it now. Thanks

@Sharathmk99
Copy link
Contributor Author

Done. Please do approve Github actions workflow, we should be good now.

Copy link
Contributor

@mihran113 mihran113 left a comment

Choose a reason for hiding this comment

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

Thanks a lot for contribution @Sharathmk99! Everything looks good now, proceeding to merge! 🎉

@mihran113 mihran113 merged commit 86839dc into aimhubio:main Jul 10, 2024
2 of 3 checks passed
@Sharathmk99
Copy link
Contributor Author

Thanks a lot for contribution @Sharathmk99! Everything looks good now, proceeding to merge! 🎉

Thank you @mihran113.

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