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

Allow self reference without link #101

Merged
merged 2 commits into from
Sep 30, 2024

Conversation

aslikaya
Copy link
Contributor

@aslikaya aslikaya commented Jul 5, 2024

Fixes #84

Comment on lines 60 to 71
let name = self
.ctx
.preamble()
.by_index(0)
.map(|field| field.name().trim())
.unwrap_or("");
let number = self
.ctx
.preamble()
.by_index(0)
.map(|field| field.value().trim())
.unwrap_or("");
Copy link
Contributor

Choose a reason for hiding this comment

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

Why have you chosen to use by_index here instead of by_name? It might make more sense to change LinkFirst to something like:

pub struct LinkFirst<S> {
    number_header: S,
    pattern: S,
}

And get the name of the field from there instead of assuming the 0-th header is always the proposal's number.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes you are right, currently it looks like a requirement but the order may be subject to change in the future and this would fail. I can change and use it like by_name("eip") but do we also need to handle if it is an erc? in that case field name will be "erc" I guess

but when I try an erc on the preamble, it gave me lint errors saying :

error[markdown-refs]: references to proposals without a `category` must use a prefix of `EIP`
  --> ../test-eipw/erc-84.md
   |
23 | ERC-84
   |
error[preamble-order]: preamble has extra header(s)
 --> ../test-eipw/erc-84.md:2:1
  |
2 | erc: 84
  | ^^^ unrecognized header
  |
  = help: see https://ethereum.github.io/eipw/preamble-order/
error[preamble-req]: preamble is missing header(s): `eip`
--> ../test-eipw/erc-84.md
 |
 |
 = help: see https://ethereum.github.io/eipw/preamble-req/

It looks like they can't get into preamble fields as an erc. What to do with ercs here? They always complicate :)

I couldn't understand why update the LinkFirst struct?

Copy link
Contributor

Choose a reason for hiding this comment

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

Both ERCs and EIPs use eip: as the preamble field.

Just to make things more confusing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed it to by_name("eip") and shortened the code slightly.
I haven't changed the LinkFirst struct yet but I can do it too

@SamWilsn SamWilsn merged commit 33a9b37 into ethereum:master Sep 30, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Self-references should not require self-links
2 participants