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.

Sign up for GitHub

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

Jump to bottom

Update EIP template: Add acceptable placeholders #5576

Merged
merged 10 commits into from
Jan 25, 2023
Merged

Update EIP template: Add acceptable placeholders #5576

merged 10 commits into from
Jan 25, 2023

Conversation

Pandapip1
Copy link
Member

  • Removes the eip preamble field from the template. Editors can simply add it themselves.
  • Moves the asterisks in the preamble fields to comments
  • Moves all the instructions to HTML comments
  • Adds a note to read EIP-1 before making an EIP
  • Removes Motivation, Reference Implementation, and Test Cases sections
  • Adds placeholders to Rationale, Backwards Compatibility, and Security considerations
  • Removes an external link

@Pandapip1 Pandapip1 requested a review from eth-bot as a code owner September 2, 2022 12:07
@github-actions github-actions bot added s-draft This EIP is a Draft t-process labels Sep 2, 2022
@eth-bot
Copy link
Collaborator

eth-bot commented Sep 2, 2022 •
edited
Loading

All tests passed; auto-merging...

(pass) eip-template.md

classification
ambiguous
  • file eip-template.md is not a valid filename, but this error has been ignored due to editor approvals

(pass) index.html

classification
ambiguous
  • file index.html is not a valid filename, but this error has been ignored due to editor approvals

xinbenlv
xinbenlv reviewed Sep 3, 2022
View reviewed changes
eip-template.md Outdated Show resolved Hide resolved
xinbenlv
xinbenlv reviewed Sep 3, 2022
View reviewed changes
eip-template.md Outdated Show resolved Hide resolved
xinbenlv
xinbenlv reviewed Sep 3, 2022
View reviewed changes
eip-template.md Outdated Show resolved Hide resolved
xinbenlv
xinbenlv reviewed Sep 3, 2022
View reviewed changes
eip-template.md Outdated

The title should be 44 characters or less. It should not repeat the EIP number in title, irrespective of the category.
<!-- Note that an EIP number will be assigned by an editor. When opening a pull request to submit your EIP, please use an abbreviated title in the filename, `eip-draft_title_abbrev.md`. -->
Copy link
Contributor

Choose a reason for hiding this comment

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

If we want to enforce the number assignment, I'd suggest the first comment to not contain any change suggestion other than change filename. I've run into many times when I propose eip-some-feature.md then feedback were given under this file, but when I change file name from eip-some-feature.md to eip-1234.md, the suggested changes no longer apply.

Copy link
Member Author

@Pandapip1 Pandapip1 Sep 3, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Good feedback - I personally would be a fan of an "@eth-bot assign" command here to avoid that very issue.

It's also why I try to avoid providing feedback until the EIP number is changed.

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure how to get around this though - send ideas here.

Copy link
Contributor

Choose a reason for hiding this comment

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

No good idea yet.

xinbenlv
xinbenlv reviewed Sep 3, 2022
View reviewed changes
eip-template.md Outdated Show resolved Hide resolved
@SamWilsn
Copy link
Contributor

I'm worried we'll get a ton of EIPs that have the comments left in with this 🤣

I do appreciate moving the optional bits after the colon in the preamble though.

@Pandapip1
Copy link
Member Author

Pandapip1 commented Sep 18, 2022 •
edited
Loading

I'm worried we'll get a ton of EIPs that have the comments left in with this 🤣

We do anyways. I'd say a good 1/8 of incoming drafts have at least one of the sections left unchanged or the bit before the abstract not removed - typically the rationale, backwards compatibility, or security considerations section.

@Pandapip1 Pandapip1 mentioned this pull request Sep 21, 2022
Closed
9 tasks
@SamWilsn
Copy link
Contributor

That's fair. Perhaps a lint for HTML style comments in eipw would help.

@Pandapip1
Copy link
Member Author

That's fair. Perhaps a lint for HTML-style comments in eipw would help.

Go ahead -- be warned that some people use them to indicate TODOs.

SamWilsn
SamWilsn reviewed Nov 4, 2022
View reviewed changes
Copy link
Contributor

@SamWilsn SamWilsn left a comment

Choose a reason for hiding this comment

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

EIP-1 links to the rendered version of the template. We should update that to the plain version (https://github.com/ethereum/EIPs/blob/master/eip-template.md?plain=1) if we use comments.

eip-template.md Show resolved Hide resolved
eip-template.md Show resolved Hide resolved
@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 Nov 22, 2022
@Pandapip1
Copy link
Member Author

Not stale.

@github-actions github-actions bot removed the w-stale Waiting on activity label Nov 23, 2022
@SamWilsn
Copy link
Contributor

Can we separate this into two PRs: one to put the instructions in comments, and then a second to remove the optional sections? I am in favour of the first, and against the second.

@Pandapip1 Pandapip1 changed the title Update EIP template: Remove non-required fields and add acceptable placeholders Update EIP template: Add acceptable placeholders Nov 25, 2022
@github-actions github-actions bot added the w-stale Waiting on activity label Dec 13, 2022
@Pandapip1
Copy link
Member Author

@SamWilsn mind re-reviewing?

@github-actions github-actions bot removed the w-stale Waiting on activity label Dec 14, 2022
@SamWilsn
Copy link
Contributor

Oh sorry, I have notifications off for this repo. Feel free to ping me directly if you need me!

SamWilsn
SamWilsn reviewed Dec 16, 2022
View reviewed changes
Copy link
Contributor

@SamWilsn SamWilsn left a comment

Choose a reason for hiding this comment

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

Sorry to throw this back with more suggestions 😅

I am not comfortable with the "if this is your first time..." style of comments. I think it makes the template somewhat hostile, plus we don't want to encourage less complete EIPs just because the contributor is new.

I split out the "this section is optional" blurbs to make them more obvious.

eip-template.md Outdated Show resolved Hide resolved
eip-template.md Outdated Show resolved Hide resolved
eip-template.md Show resolved Hide resolved
eip-template.md Outdated Show resolved Hide resolved
eip-template.md Outdated Show resolved Hide resolved
eip-template.md Outdated Show resolved Hide resolved
@Pandapip1 @SamWilsn
Apply suggestions from code review
87535c4 
Co-authored-by: Sam Wilson <57262657+SamWilsn@users.noreply.github.com>
@Pandapip1 Pandapip1 requested review from SamWilsn and xinbenlv and removed request for xinbenlv December 21, 2022 18:04
@github-actions
Copy link

github-actions bot commented Jan 5, 2023

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 Jan 5, 2023
@Pandapip1
Copy link
Member Author

@SamWilsn this is ready for review again.

@github-actions github-actions bot removed the w-stale Waiting on activity label Jan 6, 2023
@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 Jan 20, 2023
@Pandapip1
Copy link
Member Author

Still would like this.

@github-actions github-actions bot removed the w-stale Waiting on activity label Jan 21, 2023
@SamWilsn SamWilsn force-pushed the Pandapip1-eip-template-trimming branch from 860c5c0 to 0ce9034 Compare January 25, 2023 18:01
@eth-bot eth-bot enabled auto-merge (squash) January 25, 2023 18:01
@SamWilsn SamWilsn disabled auto-merge January 25, 2023 18:02
@SamWilsn SamWilsn merged commit d005d7c into master Jan 25, 2023
@SamWilsn SamWilsn deleted the Pandapip1-eip-template-trimming branch January 25, 2023 18:02
@github-actions
Copy link

The commit 0ce9034 (as a parent of d14c735) contains errors.
Please inspect the Run Summary for details.

@github-actions github-actions bot added the w-ci Waiting on CI to pass label Jan 25, 2023
iseriohn pushed a commit to iseriohn/EIP-NFT-Rights-Management that referenced this pull request Feb 16, 2023
@Pandapip1 @xinbenlv
@SamWilsn @iseriohn
Update EIP template: Add acceptable placeholders (ethereum#5576)
3908be1 
* Update EIP template: Remove non-required fields and add acceptable placeholders

* Update eip-template.md

* Update eip-template.md

Co-authored-by: xinbenlv <zzn-github@zzn.im>

* Update eip-template.md

* Add Test Cases and Reference Implementation sections back

* Add Motivation section back

* Apply suggestions from code review

Co-authored-by: Sam Wilson <57262657+SamWilsn@users.noreply.github.com>

* Small tweaks

Co-authored-by: xinbenlv <zzn-github@zzn.im>
Co-authored-by: Sam Wilson <57262657+SamWilsn@users.noreply.github.com>
Co-authored-by: Sam Wilson <sam@binarycake.ca>
@on-theway
Copy link

test

@Findmycrypto Findmycrypto mentioned this pull request Oct 19, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xinbenlv xinbenlv xinbenlv left review comments

@eth-bot eth-bot eth-bot approved these changes

@SamWilsn SamWilsn SamWilsn approved these changes

Assignees
No one assigned
Labels
s-draft This EIP is a Draft t-process w-ci Waiting on CI to pass
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Pandapip1 @eth-bot @SamWilsn @on-theway @xinbenlv