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

Remove line links from ff/README #584

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Remove line links from ff/README #584

wants to merge 4 commits into from

Conversation

mmaker
Copy link
Member

@mmaker mmaker commented Jan 21, 2023

Line links are difficult to maintain (and currently most of them are wrong). Removing them and improving the documentation with same links.

Line links are difficult to maintain (and currently most of them are wrong). Removing them and improving the documentation with same links.
@mmaker mmaker requested review from a team as code owners January 21, 2023 12:56
@mmaker mmaker requested review from Pratyush, mmagician and weikengchen and removed request for a team January 21, 2023 12:56
Copy link
Member

@Pratyush Pratyush left a comment

Choose a reason for hiding this comment

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

Thanks, I agree this is a good change! Can you check what the links point to when compiled to docs via cargo doc?

@mmaker
Copy link
Member Author

mmaker commented Jan 21, 2023

hmm it translates into broken links both with prefixed / and without.

@Pratyush
Copy link
Member

Pratyush commented Jan 21, 2023

Hm in that case there's three options:
(a) link to the docs.rs pages for these traits/structs. e.g. https://docs.rs/ark-ff/0.3.0/ark_ff/fields/trait.Field.html (using the old version because of #587)
(b) provide a github link to the file containing these traits (but not the link)
(c) just skip the links entirely

@mmaker
Copy link
Member Author

mmaker commented Jan 21, 2023

good point, going for (a)

@Pratyush
Copy link
Member

Pratyush commented Jan 21, 2023

Great, thanks! You can see the latest commit in #577 (f22cd4a) for an example where I do this. The reason I do this is because otherwise the links break on github =(

@mmaker
Copy link
Member Author

mmaker commented Jan 21, 2023

right, even with the latest commit I see how that breaks the readme..
maybe we should split the doc?

@mmaker
Copy link
Member Author

mmaker commented Jul 29, 2023

ping. @Pratyush how about leaving the readme alone?

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.

2 participants