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 Rust doc shortcodes from new book #272

Merged
merged 16 commits into from
Feb 12, 2022

Conversation

alice-i-cecile
Copy link
Member

@alice-i-cecile alice-i-cecile commented Jan 13, 2022

Problem

Solution

  • Remove short codes
  • Replace short codes with reference-style links
  • Remove shortcodes from Welcome chapter

@alice-i-cecile alice-i-cecile mentioned this pull request Jan 13, 2022
14 tasks
@alice-i-cecile alice-i-cecile changed the base branch from master to new-book January 18, 2022 19:33
@alice-i-cecile alice-i-cecile marked this pull request as ready for review January 19, 2022 01:10
@NiklasEi
Copy link
Member

This would be perfect to combine with a link check in CI as we have it on the main repo.
https://github.com/bevyengine/bevy/blob/main/.github/workflows/ci.yml#L167

@alice-i-cecile
Copy link
Member Author

Yep. I ended up having to remove a few links here because they were dead :(

Copy link
Contributor

@IceSentry IceSentry left a comment

Choose a reason for hiding this comment

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

I had a quick look around locally by clicking on a bunch of random links and everything seems to work.

The ease of reading the raw text is so much nicer!

content/learn/book/migration-guides/0.5-0.6/_index.md Outdated Show resolved Hide resolved
@@ -82,9 +85,11 @@ In order to use foreign types as components, wrap them using the newtype pattern
struct Cooldown(std::time::Duration);
```

[`Component`]: https://docs.rs/bevy/latest/bevy/ecs/component/trait.Component.html
Copy link
Contributor

Choose a reason for hiding this comment

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

Same thing here about pointing to 0.6

Copy link
Member

@cart cart left a comment

Choose a reason for hiding this comment

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

Ok this is more compelling than I expected.

Things I like:

  • We still get special formatting (I didn't know you could use code blocks as link aliases)
  • We can reuse link declarations
  • Legibility of prose goes way up
  • Easier to run deadlink checks

Thinks I dislike:

  • Less ergonomic for single-use links, especially if we did ergo improvements for short codes (default to no_mod, shorter short-code name, full paths as variable input). The proposed approach requires repeating the symbol name, repeating the crate name, specifying the url prefix (https://docs.rs/), and adding the .html suffix.
  • We're throwing away our ability to theme based on symbol type (ex: match struct and trait colors to our syntax highlighting theme) and to add more complicated html (ex: show docs on hover, show full path on hover, etc). Not the biggest deal in the world atm because we don't do either of those things, but it does feel like a loss.

However since we've agreed that our ideal code ref situation would require a preprocessor (which in turn, would resolve the issues above), I think I'm cool with moving in this direction in the interim. Moving to a preprocessor would require rewriting shortcodes anyway.

content/learn/book/migration-guides/0.5-0.6/_index.md Outdated Show resolved Hide resolved
@alice-i-cecile
Copy link
Member Author

@cart links have been re-added now! There's three links from the 0.1 announcement that are unfixable: they aren't part of the public API and those links were always dead. IMO we should just leave them unlinked at this point.

@cart
Copy link
Member

cart commented Feb 11, 2022

Those types are all exported in the public api of 0.1.0. I provided links in each comment.

@alice-i-cecile
Copy link
Member Author

Thanks @cart; I missed them because they weren't in the bevy crate itself. This should be ready-to-merge now.

@cart
Copy link
Member

cart commented Feb 12, 2022

I think the "fix missing Bevy 0.1 links" commit doesn't actually do that.

@alice-i-cecile
Copy link
Member Author

Oops; my linter went beserk. Let me try again...

@cart
Copy link
Member

cart commented Feb 12, 2022

bors r+

@cart
Copy link
Member

cart commented Feb 12, 2022

LOL

@cart
Copy link
Member

cart commented Feb 12, 2022

No bors here

@cart cart merged commit 446d3db into bevyengine:new-book Feb 12, 2022
@alice-i-cecile alice-i-cecile deleted the down-with-shortcodes branch February 12, 2022 06:49
bors bot pushed a commit that referenced this pull request Mar 11, 2022
- Fixes #207 

# Status

- [x] Split the PR
- [x] Merge #272 (do first to reduce dumb merge conflicts).
- [x] Fix all mdlint violations
- [x] Update CI to target `master` rather than `main` (or fix the branch name per #140 )
- [x] Add local rules to ignore the same lints to improve contributor experience. I followed [these instructions](https://github.com/DavidAnson/vscode-markdownlint#configure); simply renaming the version in the .github folder didn't resolve the problem for me.
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.

Replacement for shortcode docs links
4 participants