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: Fix outdated links #5706

Merged
merged 2 commits into from
Sep 26, 2022
Merged

Update EIP-1: Fix outdated links #5706

merged 2 commits into from
Sep 26, 2022

Conversation

donBarbos
Copy link
Contributor

updated invalid links to new paths

@donBarbos donBarbos requested a review from eth-bot as a code owner September 22, 2022 10:04
@github-actions github-actions bot added c-update Modifies an existing proposal t-process labels Sep 22, 2022
@eth-bot
Copy link
Collaborator

eth-bot commented Sep 22, 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

@Pandapip1 Pandapip1 changed the title Update eip-1.md Update EIP-1: Fix outdated links Sep 22, 2022
Pandapip1
Pandapip1 previously approved these changes Sep 22, 2022
@Pandapip1 Pandapip1 added this to the Manual Merge Queue milestone Sep 22, 2022
@Pandapip1 Pandapip1 added the e-review Waiting on editor to review label Sep 22, 2022
EIPS/eip-1.md Outdated Show resolved Hide resolved
xinbenlv
xinbenlv previously approved these changes Sep 22, 2022
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.

LGTM other than suggest using link to a specific commit when possible.

@xinbenlv, I didn't quite understand what you meant
like this?
@donBarbos donBarbos dismissed stale reviews from xinbenlv, ghost , and Pandapip1 via 3c43b2b September 23, 2022 14:40
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.

For the sake of not dragging this out, you don't have to make @xinbenlv's change. It would certainly be preferred, but it's probably best to just stop linking to nonexistent pages in the first place.

@SamWilsn SamWilsn merged commit 1b3dea5 into ethereum:master Sep 26, 2022
@xinbenlv
Copy link
Contributor

xinbenlv commented Sep 26, 2022

@Pandapip1 personally, I feel these links being touched are helpful. IMHO Updating the links as opposed to removing the links is great. The links exist to give references to the content and I think they are all proper links. (cc: @gcolvin @lightclient)

@donBarbos here is a little context that editors are debating whether linking to external links shall be allowed or to what extent. See #5597. You might (and IMHO, are encouraged to) make your judgement about whether links are helpful and should continue to exist in EIP-1.

@Pandapip1
Copy link
Member

@Pandapip1 personally, I feel these links being touched are helpful. IMHO Updating the links as opposed to removing the links is great. The links exist to give references to the content and I think they are all proper links. (cc: @gcolvin @lightclient)

I was agreeing. I was just saying that a speedy merge was preferable to not making any changes.

nachomazzara pushed a commit to nachomazzara/EIPs that referenced this pull request Jan 13, 2023
* Update eip-1.md

* Update eip-1.md

@xinbenlv, I didn't quite understand what you meant
like this?
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-review Waiting on editor to review t-process
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants