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: Suggest that EIPs use RFC 8174 (updates RFC 2119) #5466

Merged
merged 14 commits into from
Oct 8, 2022

Conversation

xinbenlv
Copy link
Contributor

RFC 2119 is updated by 8174. Have we discussed whether or not to adopt 8174?

RFC 2119 is updated by 8174. Have we discussed whether or not to adopt 8174?
@eth-bot
Copy link
Collaborator

eth-bot commented Aug 16, 2022

Hi! I'm a bot, and I wanted to automerge your PR, but couldn't because of the following issue(s):


(fail) eip-1.md

classification
updateEIP

(fail) eip-template.md

classification
ambiguous
  • 'eip-template.md' must be in eip-###.md format; this error will be overwritten upon relevant editor approval

@Pandapip1
Copy link
Member

The EIP template should be updated if this change is made.

@xinbenlv
Copy link
Contributor Author

xinbenlv commented Aug 18, 2022

@Pandapip1 updated eip-template.md

Now

  1. Request to remove label waiting: EIP number
  2. Request for review and merge

@Pandapip1
Copy link
Member

Request to remove label waiting: EIP number

Can't do it, it's automated. We're working on a fix.

Request for review and merge

👍

@xinbenlv
Copy link
Contributor Author

If there is no objection, and we have not fixed the bot yet, I'd suggest a manual merge so new EIPs can start using RFC 8174

EIPS/eip-1.md Outdated Show resolved Hide resolved
eip-template.md Outdated Show resolved Hide resolved
xinbenlv and others added 2 commits August 19, 2022 08:28
Co-authored-by: Pandapip1 <45835846+Pandapip1@users.noreply.github.com>
Co-authored-by: Pandapip1 <45835846+Pandapip1@users.noreply.github.com>
Pandapip1
Pandapip1 previously approved these changes Aug 19, 2022
Copy link
Member

@Pandapip1 Pandapip1 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 Pandapip1 changed the title EIP-1: propose to adopt RFC 8174 (updates 2119) Update EIP-1: Suggest that EIPs use RFC 8174 (an extension of RFC 2119) Aug 28, 2022
@xinbenlv
Copy link
Contributor Author

xinbenlv commented Sep 6, 2022

Is there a reason you don't update the actual RFC number in the anchor?

Which anchor? Could you share the screenshot of what you refer to?

@xinbenlv
Copy link
Contributor Author

xinbenlv commented Sep 6, 2022

Can we at least move it to an endnote? It's just clutter when dropped into the middle of the most important part of an EIP.

@gcolvin
I agree with you. I am just not sure markdown syntax supports endnote as well as LaTex etc. Would it be ok to you if I update it in a followup PR? We can update the policy in EIP-1 first. In a follow up PR we can address the formatting of endnote or footnote in the eip-template.md without all editors' approval, I am assuming?

@SamWilsn SamWilsn mentioned this pull request Sep 12, 2022
@SamWilsn
Copy link
Contributor

GFM supports footnotes like this[^1]

[^1]: hello

GFM supports footnotes like this1

Footnotes

  1. hello

@Pandapip1
Copy link
Member

GFM supports footnotes like this

Does CommonMark?

@xinbenlv
Copy link
Contributor Author

I'm generally OK with this, but if would cause a less clutter as a footnote. I'd be even happier to make this a general policy that applies to all EIPs, and doesn't need to be repeated in every one of them.

Got it. Do you mind I get the policy approved (EIP-1) first, and then I can work on the footnoting on a separate PRs e.g. updating the template, which requires less editor approval I'm assuming?

@xinbenlv
Copy link
Contributor Author

@axic @lightclient IIUC this proposal require your editor approval too. Could you share your feedback or approval?

lightclient
lightclient previously approved these changes Oct 3, 2022
Copy link
Member

@lightclient lightclient left a comment

Choose a reason for hiding this comment

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

LGTM

@lightclient
Copy link
Member

Are we going to update all the EIPs that only refer to RFC 2119?

@xinbenlv
Copy link
Contributor Author

xinbenlv commented Oct 4, 2022

Are we going to update all the EIPs that only refer to RFC 2119?

@lightclient
I don't think it's necessary to update all existing EIPs.

I think we can just update the template so new EIPs will gradually adopt it. Also RFC 2119 or RFC 8174 are both recommendations not requirements so if some author want to opt-out they shall have the freedom to do so. (even EIP-1 itself didn't adopt it as far as I know.)

@xinbenlv
Copy link
Contributor Author

xinbenlv commented Oct 4, 2022

Friendly ping @axic for last approval

@xinbenlv
Copy link
Contributor Author

xinbenlv commented Oct 4, 2022

@gcolvin also need your approval.
I will move it to footnote in a separate PR addressing the format on eip-template.md if that's the only concern from you.

@Pandapip1
Copy link
Member

Are we going to update all the EIPs that only refer to RFC 2119?

This is only a suggestion. EIPs are free to use or not use it. I know the general gist of RFC 8174 and it seems like generally a good idea, but requiring it for past EIPs would actually be a normative change in some cases.

@xinbenlv xinbenlv dismissed stale reviews from lightclient, ghost , SamWilsn, and Pandapip1 via 3caa94e October 5, 2022 14:45
@xinbenlv
Copy link
Contributor Author

xinbenlv commented Oct 5, 2022

Discussed EIPIP meeting 66 and editors on the call @SamWilsn @lightclient are ok to merge this PR with update that removes links from eip-template.md for now until EIPW updates.

EIPS/eip-1.md Outdated Show resolved Hide resolved
EIPS/eip-1.md Outdated Show resolved Hide resolved
EIPS/eip-1.md Outdated Show resolved Hide resolved
Co-authored-by: Pandapip1 <45835846+Pandapip1@users.noreply.github.com>
@xinbenlv
Copy link
Contributor Author

xinbenlv commented Oct 8, 2022

Thanks @SamWilsn, LGTM

@SamWilsn SamWilsn merged commit 17e3bea into ethereum:master Oct 8, 2022
nachomazzara pushed a commit to nachomazzara/EIPs that referenced this pull request Jan 13, 2023
…reum#5466)

* EIP-1: propose to adopt RFC 8174 (updates 2119)

RFC 2119 is updated by 8174. Have we discussed whether or not to adopt 8174?

* Update eip-template.md

* Update eip-template.md

* Update eip-1.md

* Update eip-template.md

* Update EIPS/eip-1.md

Co-authored-by: Pandapip1 <45835846+Pandapip1@users.noreply.github.com>

* Update eip-template.md

Co-authored-by: Pandapip1 <45835846+Pandapip1@users.noreply.github.com>

* Update eip-1.md

* Update eip-template.md

* Update eip-1.md

* Apply suggestions from code review

Co-authored-by: Pandapip1 <45835846+Pandapip1@users.noreply.github.com>

Co-authored-by: Pandapip1 <45835846+Pandapip1@users.noreply.github.com>
Co-authored-by: Sam Wilson <57262657+SamWilsn@users.noreply.github.com>
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-number Waiting on EIP Number assignment s-draft This EIP is a Draft t-process
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants