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

Added EOF prefix to 4530 and 4200, for ease of searching for EOF-related EIPs. #5849

Closed
wants to merge 1 commit into from
Closed

Conversation

bobsummerwill
Copy link

Minor formatting tweak:
Added EOF prefix to 4530 and 4200, for ease of searching for EOF-related EIPs.

@github-actions github-actions bot added c-update Modifies an existing proposal s-review This EIP is in Review t-core labels Oct 28, 2022
@eth-bot
Copy link
Collaborator

eth-bot commented Oct 28, 2022

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


(fail) eip-3540.md

classification
updateEIP

(fail) eip-4200.md

classification
updateEIP

@@ -1,6 +1,6 @@
---
eip: 3540
title: EVM Object Format (EOF) v1
title: EOF - EVM Object Format (EOF) v1
Copy link
Contributor

@xinbenlv xinbenlv Nov 10, 2022

Choose a reason for hiding this comment

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

This seems duplicating.

Is this what you intend?

Suggested change
title: EOF - EVM Object Format (EOF) v1
title: EOF - EVM Object Format v1

Copy link
Author

Choose a reason for hiding this comment

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

It is slightly redundant in this case, but the goal is to have consistent prefixing for ease of searching.

In this case ...

EOF: EVM Object Format (EOF) v1

Is fine, imho, as the two EOFs in that title serve different purposes.

Anyway - better as a new PR from @chfast, I think, to redo them all.

Copy link
Contributor

@xinbenlv xinbenlv left a comment

Choose a reason for hiding this comment

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

Thanks for cleaning and formatting. Please fix linters and this PR shall be good to go

@lightclient
Copy link
Member

@xinbenlv we will be waiting until the authors approve this change.

@xinbenlv
Copy link
Contributor

xinbenlv commented Nov 11, 2022

@lightclient oh yes, you are right. Thanks for reminder!

Copy link
Member

@chfast chfast left a comment

Choose a reason for hiding this comment

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

Please also rebase. I believe we fixed all EIP bot issues.

@@ -1,6 +1,6 @@
---
eip: 3540
title: EVM Object Format (EOF) v1
title: EOF - EVM Object Format (EOF) v1
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
title: EOF - EVM Object Format (EOF) v1
title: EOF: EVM Object Format v1

Let's use "EOF:" prefix.

Copy link
Member

Choose a reason for hiding this comment

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

EIP Walidator disagrees

Error: error[preamble-re-title-colon]: preamble header `title` should not contain `:`
 --> EIPS/eip-3540.md:3:7
  |
3 | title: "EOF: EVM Object Format v1"
  |       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ prohibited pattern was matched
  |
  = info: the pattern in question: `:`

#6007

Copy link
Member

Choose a reason for hiding this comment

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

Let's keep - then

@@ -1,6 +1,6 @@
---
eip: 4200
title: Static relative jumps
title: EOF - Static relative jumps
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
title: EOF - Static relative jumps
title: EOF: Static relative jumps

@bobsummerwill
Copy link
Author

@chfast Going with a consistent EOF: prefix makes sense to me, but will bring more files into this.

Given that the author needs to approve edits, and you are author for them all, and rebasing is needed, maybe easier to ditch this pull request and you submit a new one yourself with them consistently formatted as you wish?

I just created the PR to try to resolve the inconsistency which made it hard to search for all EOF related EIPs.

@gumb0
Copy link
Member

gumb0 commented Nov 22, 2022

EIPs were renamed, this can ba closed now.

@bobsummerwill
Copy link
Author

Thanks guys!

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 s-review This EIP is in Review t-core
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants