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

Proposal: Make EIPW only check changed lines except for changing status #7198

Closed
xinbenlv opened this issue Jun 20, 2023 · 9 comments
Closed
Labels
enhancement w-stale Waiting on activity

Comments

@xinbenlv
Copy link
Contributor

xinbenlv commented Jun 20, 2023

This was shared on: https://ethereum-magicians.org/t/proposal-eipw-should-only-complain-about-changing-lines/14762

Proposed Change

Currently some Author Pain comes from the EIPW complains about lint errors that were not introduced in that specific pull request they are working on, such as

  1. links that were not allowed
  2. mandates to call ERC "ERC" or call ERC "EIP"
  3. spaces between section
  4. order of section

In particular, sometimes new policy will be imposed on EIP, and it means many existing EIP are in the state of inconsistency.

This is particular confusing and painful which requires Authors to get up to date to EIP editorial policy.

I hereby propose a change to EIPW:

  1. For any PR that's not changing state or Draft/Review, only lint the line of code being changed.
  2. Only lint full EIP when moving into Last Call or Final

This was inspired by https://twitter.com/sproulM_/status/1671096282482622464?s=20

@xinbenlv xinbenlv changed the title Proposal: Make EIPW only check changed lines - Proposal: Make EIPW only check changed lines except for Finalizing or Last Call Jun 20, 2023
@xinbenlv
Copy link
Contributor Author

Created #7199

@Pandapip1
Copy link
Member

I would be okay with the following:

  • eipw will always output an error for last call EIPs
  • eipw will always output an error for status change PRs
  • Otherwise, eipw will output warnings (non-blocking annotations)

@xinbenlv
Copy link
Contributor Author

I see, it seems the only difference is whether small change to Last Call will be required to fix all errors, right?

@Pandapip1
Copy link
Member

No. An additional requirement is that all status changes (not just review -> last call or last call -> final) also proc eipw.

@xinbenlv
Copy link
Contributor Author

Oh, I see. I am ok all status change commits require error-clean. Let me update this proposal

@xinbenlv
Copy link
Contributor Author

@Pandapip1 agree with you. This update is now reflected in #7199 , can you see if it's good to merge as a draft?

@xinbenlv xinbenlv changed the title Proposal: Make EIPW only check changed lines except for Finalizing or Last Call Proposal: Make EIPW only check changed lines except for changing status Jun 22, 2023
@gcolvin
Copy link
Contributor

gcolvin commented Jun 23, 2023

Yes, please. I've been bitten by this myself more than once.

Copy link

github-actions bot commented Nov 1, 2023

There has been no activity on this issue for 1 week. It will be closed after 3 months of inactivity.

@github-actions github-actions bot added the w-stale Waiting on activity label Nov 1, 2023
Copy link

This issue was closed due to inactivity. If you are still pursuing it, feel free to reopen it and respond to any feedback.

@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Dec 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement w-stale Waiting on activity
Projects
None yet
Development

No branches or pull requests

4 participants
@xinbenlv @gcolvin @Pandapip1 and others