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

Update EIP-1: Strengthen wording of status update PRs #6894

Closed
wants to merge 4 commits into from

Conversation

Pandapip1
Copy link
Member

Why stick to final EIPs? It makes sense to require this for all status changes.

Why use SHOULD when we can use MUST?

Requiring the latest version of an EIP to review seems like a pretty obvious request.

@Pandapip1 Pandapip1 requested a review from eth-bot as a code owner April 17, 2023 17:55
@github-actions github-actions bot added c-update Modifies an existing proposal t-process labels Apr 17, 2023
@eth-bot
Copy link
Collaborator

eth-bot commented Apr 17, 2023

File EIPS/eip-1.md

Requires 2 more reviewers from @axic, @gcolvin, @lightclient, @SamWilsn

@eth-bot eth-bot added the e-consensus Waiting on editor consensus label Apr 17, 2023
EIPS/eip-1.md Outdated Show resolved Hide resolved
Copy link
Contributor

@g11tech g11tech left a comment

Choose a reason for hiding this comment

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

sounds fine to me

@SamWilsn
Copy link
Contributor

SamWilsn commented Apr 19, 2023

I am rather torn about this. I like being able to see my comments and suggestions applied to the same PR that I made them on. Makes it easier to track progress.

I'd be less opposed if we can enforce that the PR's "Files Changed" tab shows the most recent version in master with the status change diff. I think that's the intent of:

When the content of an EIP changes, the git branch of any PR that changes the status of that EIP MUST be updated to reflect the current version.

Is it easy to automatically enforce this rule? I think there's an option in GitHub to require PRs be up-to-date, but that would require all PRs follow that rule.

@g11tech
Copy link
Contributor

g11tech commented Apr 19, 2023

I'd be less opposed if we can enforce that the PR's "Files Changed" tab shows the most recent version in master with the status change diff. I think that's the intent of:

I think thats how it normally works, the target/master branch gets auto updated if something is merged (and if there are conflicts they show up)

Is it easy to automatically enforce this rule? I think there's an option in GitHub to require PRs be up-to-date, but that would require all PRs follow that rule.

i think yes, for e.g. in ethereumjs repo we follow similar rule, however github also shows up with a button to rebase the PR with latest target/master at which then the PR becomes mergable

Co-authored-by: g11tech <develop@g11tech.io>
@Pandapip1 Pandapip1 requested a review from g11tech April 24, 2023 20:14
Copy link
Contributor

@g11tech g11tech left a comment

Choose a reason for hiding this comment

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

lgtm!

@Pandapip1
Copy link
Member Author

Pandapip1 commented Apr 27, 2023

I think there's an option in GitHub to require PRs be up-to-date, but that would require all PRs follow that rule.

It's easy to use the API to automatically rebase relevant PRs on push to the EIPs repository.

The only reason I am hesitant to use rebases as opposed to merges is that it requires a force pull, which makes me very unhappy.

@g11tech
Copy link
Contributor

g11tech commented Apr 30, 2023

i think auto magic could be very non intutive to the author who might want to pull/push exactly for the reasons that you mentioned.
However a force push and update by author(s) of PR is still ok and is not really a big ask.

We just need to block the PRs saying they need rebase as the PR is out of touch, this should also provide a signal to the author to re-review (or may be we can explicitly ask the author to rebase and review)

@github-actions
Copy link

There has been no activity on this pull request for 2 weeks. It will be closed after 3 months of inactivity. If you would like to move this PR forward, please respond to any outstanding feedback or add a comment indicating that you have addressed all required feedback and are ready for a review.

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

This pull request was closed due to inactivity. If you are still pursuing it, feel free to reopen it and respond to any feedback or request a review in a comment.

@github-actions github-actions bot closed this Oct 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c-update Modifies an existing proposal e-consensus Waiting on editor consensus t-process w-stale Waiting on activity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants