Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
started the burn developer book #1184
started the burn developer book #1184
Changes from 5 commits
260f93b
d9a637f
8fc11bb
c1b4dae
e2738a8
79c29c6
cc93bab
fb64d11
bcc232e
227a5ea
9ea4498
15c0212
f408eff
b4f84d0
151f444
8c67766
b312607
5ac299a
c4ee85b
f63a79d
07db8ad
1f89007
20057bf
f0ba1d2
418a08a
3ea3219
2739781
5d06209
726d232
e9ee469
c1f6304
b3e84ef
6d9c783
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
We have a new Xtask command to manage our books, for instance
cargo xtask books burn open
. It will install the required cargo crates automatically. We should consolidate all our commands behindcargo xtask
.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.
That's hard to read.
Also I would recommend to avoid pointing to lines of code because the code base will evolve and we will end up with broken links. I suggest to point to files only and then use struct names to point a specific part in the file.
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.
actually, all the links are permalinks, as in line numbers on specific commits. Those should only be changed if the code undergoes changes so significant that the specific section would need to be rewritten
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.
Also what do you mean it's hard to read? do you mean the structure of the text is hard to understand or the unrendered markdown links are hard to read?
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.
I had to reread it several times but I was not using the rendered files indeed 😆. While those links are locked to a specific commit I believe it is better to make the documentation more resilient to the changes by avoiding the necessity to link to the code base.
Also a trick to avoid inlining HTTP links in the paragraphs is to write them at the end of the file. They become
[the burn-tensor crate]()
in the text and then at the end of the file we have[the burn-tensor crate]: https://...
. This link can also be reused easily in the same file making it easier to update.Here is an example of how we can avoid being too close to the source code:
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.
for sections like the architecture, that's probably a good idea, though in the case of guides for contributions that involve a significant amount of changes across the project I disagree. take the case of the op addition. Some of the files linked are a thousand+ lines, and may involve changes in multiple places. Plus, IMHO it's generally better to lean towards making instructions as explicit and unambiguous as possible. It's easy to scan over a file and miss something, I definitely did a few times.
I'm currently using that for the footnotes, generally for useful information or tips not directly related to burn.
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.
Fine with me. But we should avoid linking to the code base when it is not strictly necessary. For instance I don't think it is really necessary to link to the exact line of the macro rule definition in the file. In a big file there can be value to those links indeed.