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

Maintenance fixes #3398

Merged
merged 2 commits into from
Sep 19, 2024
Merged

Maintenance fixes #3398

merged 2 commits into from
Sep 19, 2024

Conversation

ordabayevy
Copy link
Member

Copy link
Collaborator

@martinjankowiak martinjankowiak left a comment

Choose a reason for hiding this comment

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

lgtm thanks!

@kit1980
Copy link
Contributor

kit1980 commented Sep 19, 2024

You can add TorchFix to the CI to prevent regressions and get future improvements.
It would be very easy if you run flake8, but I see you recently removed flake8 from the CI...

@ordabayevy ordabayevy merged commit 88ae262 into dev Sep 19, 2024
9 checks passed
@ordabayevy ordabayevy deleted the maintenance-fixes branch September 19, 2024 20:41
@ordabayevy
Copy link
Member Author

Yes, we moved from flake8 to ruff because it is much faster. Do you plan to support ruff in the future?

@kit1980
Copy link
Contributor

kit1980 commented Sep 19, 2024

For ruff everything needs to be rewritten completely, in Rust. Maybe someone from ruff community will do that.
TorchFix already uses LibCST as the main library, which is written in Rust, so should be fast already.
Maybe you want to re-enable flake8 just for TorchFix?

Also there is a standalone torchfix script besides the flake8 interface, but currently it doesn't have support for running in CI.

@martinjankowiak
Copy link
Collaborator

thanks @kit1980 !

we probably don't want to bring flake8 back into our CI, but i've made an issue to track this in case future versions of torchfix can be brought into our CI more easily: #3399

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants