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

ci/lint: fix calling Ruff's format #1457

Merged
merged 3 commits into from May 2, 2024
Merged

Conversation

Borda
Copy link
Contributor

@Borda Borda commented Apr 29, 2024

It seems that Ruff'f format was not called correctly; in fact, as both Ruff's IDs had overwritten entry, it was still calling check with a confusion of id: ruff-format

Found based on #1425 (comment)

See the original entry call:
https://github.com/astral-sh/ruff-pre-commit/blob/596470fba20d04adc68ec7903ff69a12e5e1a8d3/.pre-commit-hooks.yaml#L15

cc: @rbren @li-boxuan

@codecov-commenter
Copy link

Welcome to Codecov 🎉

Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests.

Thanks for integrating Codecov - We've got you covered ☂️

Copy link
Collaborator

@li-boxuan li-boxuan left a comment

Choose a reason for hiding this comment

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

LGTM, but it's gonna be a huge pain for people to handle merge conflicts :(

@Borda
Copy link
Contributor Author

Borda commented Apr 30, 2024

LGTM, but it's gonna be a huge pain for people to handle merge conflicts :(

Yeah, only other way is drop false hook with "formatting"

@rbren
Copy link
Collaborator

rbren commented Apr 30, 2024

@Borda I'm generally a fan of enforcing new style changes one by one as they change--is that possible here?

@Borda
Copy link
Contributor Author

Borda commented Apr 30, 2024

I'm generally a fan of enforcing new style changes one by one as they change--is that possible here?

@rbren Not sure what you mean, this is Black.

@li-boxuan
Copy link
Collaborator

li-boxuan commented Apr 30, 2024

I'm generally a fan of enforcing new style changes one by one as they change--is that possible here?

It should be possible. We just need to run pre-commit only on changed files (i.e. on diff), not all files.

@Borda the ask is, can we avoid linting all files in this PR? This is a huge PR that will cause a lot of open PRs to have merge conflicts.

@Borda
Copy link
Contributor Author

Borda commented Apr 30, 2024

can we avoid linting all files in this PR? This is a huge PR that will cause a lot of open PRs to have merge conflicts.

Yes, we can, just to be aware that many PRs will include unrelated changes to the PR's topic...
For example, simple PR with fixing typo may suddenly include another 50 changes

@li-boxuan
Copy link
Collaborator

For example, simple PR with fixing typo may suddenly include another 50 changes

I think that's fine. It definitely reduces the burden for developers. People hate merge conflicts and if they hate it too much, they will start to hate all linting efforts.

@Borda
Copy link
Contributor Author

Borda commented May 1, 2024

@li-boxuan @rbren dropped applied pre-commit but noted that the lining job will fail...

@iFurySt
Copy link
Collaborator

iFurySt commented May 1, 2024

pre-commit run --files $(git diff --name-only $(git merge-base main $(git branch --show-current)) $(git branch --show-current) | tr '\n' ' ') ...

this method can ensure only checking the modified files. but it is a bit troublesome to apply to workflow. it needs to fetch the main and current branch and unshallow clone~

@iFurySt
Copy link
Collaborator

iFurySt commented May 1, 2024

@rbren @li-boxuan what do you think of this method to transition?

Copy link
Collaborator

@rbren rbren left a comment

Choose a reason for hiding this comment

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

LGTM!

@iFurySt
Copy link
Collaborator

iFurySt commented May 1, 2024

LGTM!

it causes the lint workflow to fail right now 🤔

@li-boxuan
Copy link
Collaborator

li-boxuan commented May 1, 2024 via email

@rbren
Copy link
Collaborator

rbren commented May 1, 2024

Ugh you're right. Not sure how we did this in the past. Maybe we do just need to delint everything in this PR and deal with the merge headaches

@li-boxuan
Copy link
Collaborator

li-boxuan commented May 1, 2024 via email

@iFurySt
Copy link
Collaborator

iFurySt commented May 1, 2024

I've pushed a patch for transition. It only checks the modified files from the commits.

@neubig neubig merged commit 1b810cf into OpenDevin:main May 2, 2024
24 checks passed
@Borda Borda deleted the precommit/format branch May 2, 2024 05:40
@rbren rbren mentioned this pull request May 2, 2024
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.

None yet

6 participants