Skip to content

Update ruff to version 0.5.2 and workflows update #892

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

Merged
merged 20 commits into from
Jul 17, 2024

Conversation

Smartappli
Copy link
Contributor

@Smartappli Smartappli commented Jul 17, 2024

Workflow updated

@qubvel
Copy link
Collaborator

qubvel commented Jul 17, 2024

Hi @Smartappli , thanks for the opening PR!
Let me know when its ready 🙂

@Smartappli
Copy link
Contributor Author

Hi @Smartappli , thanks for the opening PR! Let me know when its ready 🙂

Hi @qubvel, its ready

@qubvel
Copy link
Collaborator

qubvel commented Jul 17, 2024

Can you please alse update ruff in setup.py?

@Smartappli Smartappli changed the title Update ruff to version 0.5.1 Update ruff to version 0.5.2 Jul 17, 2024
@Smartappli Smartappli changed the title Update ruff to version 0.5.2 Update ruff to version 0.5.2 and workflows update Jul 17, 2024
@qubvel
Copy link
Collaborator

qubvel commented Jul 17, 2024

One more note here, can you please pin timm version back? It's going to be updated in another PR, thanks.

@Smartappli
Copy link
Contributor Author

@qubvel ready to merge

Copy link
Collaborator

@qubvel qubvel 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 working on this, a few more comments below. Moreover, CI fails due to changed ruff configuration

@Smartappli Smartappli requested a review from qubvel July 17, 2024 13:57
@qubvel
Copy link
Collaborator

qubvel commented Jul 17, 2024

Sorry, probably there was a kind of misunderstanding, I thins exclude is necessary, but I asked about what was the motivation to add ruff.toml?

@Smartappli
Copy link
Contributor Author

Smartappli commented Jul 17, 2024

Sorry, probably there was a kind of misunderstanding, I thins exclude is necessary, but I asked about what was the motivation to add `ruff.toml?

Its
more easy to.configure.ruff.with a toml.than directly in the yml.

@qubvel
Copy link
Collaborator

qubvel commented Jul 17, 2024

Sorry, I cannot merge it this way because it breaks CI. I suggest moving ruff config change to a separate PR and keeping here just ruff version change to be able to pass the CI checks.

In next PR the config might be created and changed, then ruff formatting applied to reflect config changes.

@Smartappli
Copy link
Contributor Author

Sorry, I cannot merge it this way because it breaks CI. I suggest moving ruff config change to a separate PR and keeping here just ruff version change to be able to pass the CI checks.

In next PR the config might be created and changed, then ruff formatting applied to reflect config changes.

reverted

Copy link
Collaborator

@qubvel qubvel left a comment

Choose a reason for hiding this comment

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

Looks good, thanks for iterating!

@qubvel qubvel merged commit f92821a into qubvel-org:main Jul 17, 2024
2 checks passed
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