-
-
Notifications
You must be signed in to change notification settings - Fork 410
fix: change all doc links to permalinks #8574
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
base: unstable
Are you sure you want to change the base?
Conversation
Summary of ChangesHello @edna-harriet, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request systematically updates all external references within the codebase, primarily focusing on links to Ethereum consensus specifications. The goal is to enhance the reliability and precision of our documentation and code comments by ensuring that all referenced specifications are linked via stable, verified SHA commits rather than potentially changing branch names. This prevents broken links and ensures that the codebase always refers to the exact version of the specification it was built against. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request focuses on updating documentation and code references to point to the most current SHA commits, replacing outdated branch references. The changes primarily involve updating .ts files and adding 'Reference' prefixes to comments that link to external resources. I have identified some areas where the addition of 'Reference' seems unnecessary and suggest removing them to maintain clarity and conciseness.
packages/beacon-node/src/chain/blocks/verifyBlocksExecutionPayloads.ts
Outdated
Show resolved
Hide resolved
packages/beacon-node/src/chain/opPools/aggregatedAttestationPool.ts
Outdated
Show resolved
Hide resolved
packages/beacon-node/src/chain/opPools/aggregatedAttestationPool.ts
Outdated
Show resolved
Hide resolved
|
Warning Gemini encountered an error creating the review. You can try again by commenting |
packages/beacon-node/src/chain/opPools/aggregatedAttestationPool.ts
Outdated
Show resolved
Hide resolved
packages/beacon-node/src/chain/opPools/aggregatedAttestationPool.ts
Outdated
Show resolved
Hide resolved
packages/beacon-node/src/chain/opPools/aggregatedAttestationPool.ts
Outdated
Show resolved
Hide resolved
|
Glad to see you getting this worked through. Per our discussion on discord DM we would prefer to use spec release version tags. I see that @nflaig has mentioned that above as well. Please use this format Also please address the Gemini comments and remove the added lines. All that is necessary is the link itself. |
packages/beacon-node/src/chain/blocks/verifyBlocksExecutionPayloads.ts
Outdated
Show resolved
Hide resolved
|
@matthewkeil @nflaig , all your feedbacks are taken in. Am working on it. there are afew references that opens with SHA commit but when i change that to release tags(from the different available versions), the page breaks-Error 404. What do you advise I do in such case? |
Links that are already using a commit sha are already permalinks, there is no need to update them |
|
@nflaig @matthewkeil , I am ready for another review. I have implemented all AI and your requested changes.Thank you. |
Noted |
packages/beacon-node/src/chain/opPools/aggregatedAttestationPool.ts
Outdated
Show resolved
Hide resolved
it doesn't seem like it, there are still extra modifications and links that are already permalinks shouldn't be updated |
|
@nflaig @matthewkeil , I worked on this and awaiting your review. |
I think you still have some review to do. Please go through the There are still a bunch of added empty lines, added words before the updated links (like |
@matthewkeil , i feel abit lost on this task. I know there are reference comments that I added that should be removed eg the one you have cited above (Reference : on_payload). how about these ones where i replaced branch name with tag? what specifically am I supposed to correct about it? Remove the //Mainnet config OR leave the link with the dev branch as it was initially? Kindly shed some clarity on it? // Mainnet config |
packages/beacon-node/src/chain/opPools/aggregatedAttestationPool.ts
Outdated
Show resolved
Hide resolved
I added a couple notes above. I hope that helps. Please also make sure to click "resolve" for the gemini created issues if they are fixed. |
|
@matthewkeil, I have read the notes but I feel confused now that you have said more diffs are incorrect. I followed your earlier suggested convention here : https://github.com/ethereum/consensus-specs/blob/v1.5.0/specs/altair/beacon-chain.md#has_flag.
By tags for permalink, did you mean : v1.5.0 or #has_flag . I focused on using for example, like v1.5.0 and seemingly I did not get it right.
Would you mind scheduling a call so that you may explain to me? I feel it's some concept about addressing this issue that I have not yet grasped.
…On Thu, Oct 30, 2025 at 7:47 PM Matthew Keil ***@***.***> wrote:
*matthewkeil* left a comment (ChainSafe/lodestar#8574)
<#8574 (comment)>
@nflaig <https://github.com/nflaig> @matthewkeil
<https://github.com/matthewkeil> , I worked on this and awaiting your
review.
I think you still have some review to do. Please go through the files
view of this PR to self audit where your work stands.
https://github.com/ChainSafe/lodestar/pull/8574/files
There are still a bunch of added empty lines, added words before the
updated links (like * Reference : on_payload) and deleted existing
comments. The bar for inclusion into the codebase is high and needs a very
detailed look. Please make sure everything is 100% before marking this as
ready for review. Once you go through each of the comments above and
resolve each, please click the resolve button on the comment.
@matthewkeil <https://github.com/matthewkeil> , i feel abit lost on this
task. I know there are reference comments that I added that should be
removed eg the one you have cited above (Reference : on_payload). how about
these ones where i replaced branch name with tag? what specifically am I
supposed to correct about it? Remove the //Mainnet config OR leave the link
with the dev branch as it was initially? Kindly shed some clarity on it?
// Mainnet config //
https://github.com/ethereum/consensus-specs/blob/dev/configs/mainnet.yaml
//
https://github.com/ethereum/consensus-specs/blob/v1.6.0-alpha.6/configs/mainnet.yaml
I added a couple notes above. I hope that helps
—
Reply to this email directly, view it on GitHub
<#8574 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACGJW7KHGBWKFFGIZQTD4XL32I6K7AVCNFSM6AAAAACKJUM76KVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZTINRZGAYDENBWGI>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
@edna-harriet all you have to do for this issue is search the code for any other modifications like extra new lines, reformatting need to be avoided |
…d been replaced with a tag
Closes issue #8171
Motivation
This PR replaces documentation and code references that previously pointed to the dev, main, master branches while also updating existing SHA commits with the release tags. However, references with SHA commits that would re-direct to broken page(Error 404) when replaced with tags, have not been changed.Those references still have their SHA commits.
Description
-Updated only .ts files containing github.com/ethereum/consensus-specs/blob/dev or main or master links.
-Also verified that no auto-generated file such as .js and .d.ts were modified.
Steps to test or reproduce
$ cd lodestar
$ git checkout issue-#8171
$ grep -Rnl "https://github.com/ethereum/consensus-specs/blob/[0-9a-f]\{7,40\}" --include='.ts' --exclude='.d.ts' --exclude='*.js' .
Conclusion
I invite review and I am ready to make request changes.