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

Book: repair links with mdbook-linkcheck (manually) #1505

Merged
merged 4 commits into from
Dec 11, 2023

Conversation

volhovm
Copy link
Member

@volhovm volhovm commented Dec 11, 2023

PR 1/2 of #1359 that for now does a manual pass over the book and fixes link inconsistencies.

(The future PR 2/2 will actually integrate the checker after Michael-F-Bryan/mdbook-linkcheck#81 merged)

@volhovm volhovm force-pushed the volhovm/1359-add-mdbook-linkcheck branch from 0fdf26b to 9c7b359 Compare December 11, 2023 09:47
@volhovm volhovm marked this pull request as ready for review December 11, 2023 09:48
@volhovm volhovm requested a review from querolita December 11, 2023 09:49
book/Makefile Outdated
cargo install "mdbook-mermaid@$(MDBOOK_MERMAID_VERSION)"
cargo install "mdbook-toc@$(MDBOOK_TOC_VERSION)"

#
# use `make check` to check if your installed dependencies match what we've listed above
# Checks if your installed dependencies match what we've listed above
Copy link
Member

Choose a reason for hiding this comment

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

what command does this now instead? If it is not done automatically, I would keep the comment just in case someone wants to check this manually. Same with the other comments that have been changed in the Makefile.

Copy link
Member Author

Choose a reason for hiding this comment

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

The same command below, make check. It's just that everything in makefile was duplicated, there was a target "mytarget" and a comment "use make mytarget to ...".

If it clarified but not confused things I can return it back?

Copy link
Member

@querolita querolita left a comment

Choose a reason for hiding this comment

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

Looks good to me, but take a look at the comment, as I wouldn't like to provide less guidance for users by removing concrete explanations.

@volhovm volhovm force-pushed the volhovm/1359-add-mdbook-linkcheck branch from 9c7b359 to 0fc9487 Compare December 11, 2023 11:23
Copy link
Member

@dannywillems dannywillems left a comment

Choose a reason for hiding this comment

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

Please do a PR with only link fixes.
make serve does not work locally because of the newly added configuration for linkcheck, which, I suppose, is fixed by your PR upstream.
It breaks local compilation of the documentation.

book/book.toml Outdated Show resolved Hide resolved
@volhovm
Copy link
Member Author

volhovm commented Dec 11, 2023

@dannywillems reverted the Makefile related changes.

@@ -2225,9 +2226,9 @@ The prover then follows the following steps to create the proof:
1. Evaluate the same polynomials without chunking them
(so that each polynomial should correspond to a single value this time).
1. Compute the ft polynomial.
This is to implement [Maller's optimization](https://o1-labs.github.io/mina-book/crypto/plonk/maller_15.html).
This is to implement [Maller's optimization](https://o1-labs.github.io/mina-book/kimchi/maller_15.html).
Copy link
Member

Choose a reason for hiding this comment

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

I would use the local version.

1. construct the blinding part of the ft polynomial commitment
[see this section](https://o1-labs.github.io/mina-book/crypto/plonk/maller_15.html#evaluation-proof-and-blinding-factors)
[see this section](https://o1-labs.github.io/mina-book/kimchi/maller_15.html#evaluation-proof-and-blinding-factors)
Copy link
Member

Choose a reason for hiding this comment

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

Same here

Copy link
Member

@dannywillems dannywillems left a comment

Choose a reason for hiding this comment

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

LGTM. Merging. You can address, if you agree with, the comments in a follow-up PR.

@dannywillems dannywillems merged commit a9888d6 into master Dec 11, 2023
4 checks passed
@dannywillems dannywillems deleted the volhovm/1359-add-mdbook-linkcheck branch December 11, 2023 17:36
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.

3 participants