search

aimhubio / aim

Aim šŸ’« ā€” An easy-to-use & supercharged open-source experiment tracker.
https://aimstack.io
Apache License 2.0
5.16k stars 316 forks source link

[refactor] Integrate Ruff for lint & format #3125

Closed Sharathmk99 closed 2 months ago

Sharathmk99 commented 6 months ago

Ruff integration for lint & format to maintain code standardisation.

Fixes - #3124 #3099

It works perfectly fine with vscode extension

Sharathmk99 commented 6 months ago

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

Sharathmk99 commented 6 months ago

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

Sharathmk99 commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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 commented 3 months ago

@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

Sharathmk99 commented 2 months ago

@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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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

Sharathmk99 commented 2 months ago

Thanks a lot for contribution @Sharathmk99! Everything looks good now, proceeding to merge! šŸŽ‰

Thank you @mihran113.