Skip to content
This repository has been archived by the owner on Feb 9, 2023. It is now read-only.

Warn about external links #73

Closed
SamWilsn opened this issue Apr 20, 2022 · 10 comments
Closed

Warn about external links #73

SamWilsn opened this issue Apr 20, 2022 · 10 comments

Comments

@SamWilsn
Copy link

SamWilsn commented Apr 20, 2022

Originally from #55

It would be nice if the bot commented on PRs that use full URLs (i.e. those starting with https/http/etc.) They are advised against in EIP-1.

This shouldn't block merging, because EIP-1 does allow for exceptions.

@Pandapip1
Copy link
Member

Exceptions:

  • Links to Ethereum-magicians should be allowed if they are in the discussions-to.
  • Links to the creative commons website should be allowed, or at least the link to cc0 should be.
  • Links to RFCs should be allowed, or at least rfc 2119 should be
  • Links to gETH PRs should be allowed.

@SamWilsn
Copy link
Author

  • Links to the creative commons website should be allowed, or at least the link to cc0 should be.

Now that we have our own copy of CC0, maybe we update the template and EIP-1 to point to that instead? If not, I fully agree!

  • Links to RFCs should be allowed, or at least rfc 2119 should be

Should they? I don't think the template links directly to it.

  • Links to gETH PRs should be allowed.

Why?

@Pandapip1
Copy link
Member

Pandapip1 commented Apr 22, 2022

  1. I thought of that right after sending (I was the one who brought up putting CC0 in the EIP repository in the first place), and already have opened an issue :)
  2. The template doesn't link to it, you're right. It probably should be, though - no harm is done.
  3. The reference implementation section for PRs that change validation can't just include the entirety of go-ethereum. What I'm seeing be done is to link to a PR on go-ethereum that's merged when the EIP is finalized: https://github.com/ethereum/EIPs/pull/5027/files#diff-cf3b9fccf15d23aa2557af69d5a5241fa222bcded0fe1a3ba09355b2dff9ca9eR78 and https://github.com/ethereum/EIPs/pull/5022/files#diff-bbe493336a7242fda30f5aae432ce0239600e70b1ae34fd01edb9d79b10efe6aR63 are two examples.

@MicahZoltu
Copy link
Contributor

Both of those examples wouldn't get past me in review anymore as we do not allow external links. I am especially hostile against external links to PRs against 3rd party products, and I'm super duper against enshrining geth above other Ethereum clients in any way.

@JEAlfonsoP
Copy link
Contributor

(I know you know it) EIP 1:

Linking to External Resources
Links to external resources SHOULD NOT be included. External resources may disappear, move, or change unexpectedly.

I believe this issue would required EIP 1 modification. If this is the case please lets close it and move it to the right repo for further discussion.

@Pandapip1
Copy link
Member

Uh, should != must. No modification to EIP-1 is necessary.

@JEAlfonsoP
Copy link
Contributor

I noticed it when I posted it. What is supposed to be done in these cases ? Should Not...
is there an erc for this kind of required consensus ? or something else?

@MicahZoltu
Copy link
Contributor

Just comment on the PR recommending the user remove the external link. No need for anything else beyond that for now.

@JEAlfonsoP
Copy link
Contributor

Roger that.

@MicahZoltu
Copy link
Contributor

EIPW addresses this.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants