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

chore(lint): use new-from-rev field #115

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

qdm12
Copy link

@qdm12 qdm12 commented Feb 4, 2025

Why this should be merged

Only lint changes made after the last base libevm commit (includes line modifications in non-libevm code)

How this works

  • new-from-rev set to the current libevm base commit hash. Remove path-except linting rules which should no longer be needed.
  • New rename-module workflow job "update-golangci" which updates the new-from-rev commit hash to the new rename module commit hash

How this was tested

  • golangci-lint run working as expected on new changes only
  • Linting CI passing
  • We should try by dispatching the rename-module workflow, but I wanted to check what we think on this first.

@ARR4N
Copy link
Collaborator

ARR4N commented Feb 4, 2025

Love the idea vs linting only the current PR. I'll delve into the specifics of what you've done once it's out of draft.

@qdm12
Copy link
Author

qdm12 commented Feb 4, 2025

The advantage of this approach: lints changed code in existing geth code (not just *libevm* paths)
The disadvantage of this approach: modified/new code before the last libevm base commit would never be linted again.

Thoughts on this?

Note I have seen multiple times a golangci-lint version upgrade return new lint errors (sometimes important too), so I'm a bit hesitant regarding the disadvantage mentioned... 😢

linting only the current PR

Done & merged in #116

@qdm12 qdm12 force-pushed the qdm12/ci/linting-new-from-rev branch 2 times, most recently from 4e1a4aa to 8021289 Compare February 5, 2025 08:13
@qdm12 qdm12 force-pushed the qdm12/ci/linting-new-from-rev branch from 8021289 to a7e8b80 Compare February 5, 2025 08:15
@ldez
Copy link

ldez commented Feb 10, 2025

I think the new option new-from-merge-base can help you.

golangci/golangci-lint#5362

This option will be available inside golangci-lint v1.64.0.

@ARR4N
Copy link
Collaborator

ARR4N commented Feb 10, 2025

Thank you @ldez

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