-
-
Notifications
You must be signed in to change notification settings - Fork 375
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
Docs tweaks #1518
Docs tweaks #1518
Conversation
- Since every changenote in a Briefcase release includes a link to the GitHub issue/PR, link checking spends most of its time verifying well-known links...so, this omits them from docs linting.
04b12df
to
8b92373
Compare
@@ -23,48 +23,60 @@ Platform support | |||
+-----+-------------------------------------+ | |||
|
|||
|
|||
.. |Gradle| replace:: **Gradle** |
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.
curious....did you intentionally use non-breaking spaces for these? I feel like you did and hence, I maintained them. Although, without them, the whole table does fit better on the page.
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.
Yes, it was intentional. Without the non-breaking space, you end up with a table where "Native System Packages" is split over 3 lines... and the table still doesn't fit on the page. I opted to have a better looking table titles, and a slightly worse fit on the page.
This comment talks with about switching to MyST. What's the blocker here? Just actually doing the work? |
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.
All looks good. I've flagged two points of interest inline, but otherwise this looks good.
As for the MyST question - yes, it's mostly a "someone needs to do it" thing, combined with the fact that I haven't actually used MyST in practice, so I'm not sure how much of what I'm seeing about MyST is grass-is-greener optimism.
ReST... works... but it has limitations. But we know what those are, and we've got workarounds when we hit them. MyST is somewhat of an unknown to me personally. There's plenty of social proof (other projects that have adopted it, including RTD). However, on the basis that all technology sucks, it's just a question of how it sucks, I'm a little wary of just switching.
That said - the other big reason to switch is that the rest of the ecosystem has very much converged on Markdown rather than ReST, and it's been made very clear that GitHub isn't going to improve it's ReST support any time soon, so switching to a Markdown-based documentation approach has a certain amount of "running downhill" associated with it.
I guess I would like to see a speculative port to see how disruptive a switch would be, how much we can automate rather than needing manual transformation, and how much we'll need to audit after an automated transformation.
If, hypothetically, someone were so inclined :-)
@@ -23,48 +23,60 @@ Platform support | |||
+-----+-------------------------------------+ | |||
|
|||
|
|||
.. |Gradle| replace:: **Gradle** |
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.
Yes, it was intentional. Without the non-breaking space, you end up with a table where "Native System Packages" is split over 3 lines... and the table still doesn't fit on the page. I opted to have a better looking table titles, and a slightly worse fit on the page.
r"^./windows/app.html$", | ||
r"^./windows/visualstudio.html$", | ||
r"^https://github.com/beeware/briefcase/issues/\d+$", | ||
r"^https://github.com/beeware/briefcase/pull/\d+$", |
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.
Not sure how I feel about this one.
On the one hand - can't argue with the practicality. The link checks on the release notes page are overwhelmingly the most lengthy part of the test (it's even worse on Toga). And, these links are automatically generated
But... I've also caught plenty of bugs with this check, because while the links are automatically generated, they're often edited - and the links are often updated from issues to pull as part of the editing process so that they don't return redirects based on the printed link.
I think it's probably worth it for the developer experience improvement... but I'll flag that I'd love a better option (I just don't know what that is).
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 always forget to note when something is "intentionally provocative" and I fully expect push back...as I did here.
The only alternative I really have in mind is setting linkcheck_workers
to something larger than five (5). It may at least run faster then.....as long as GitHub doesn't start returning 429s or something.
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.
Move fast and break things, right? :-)
In this case, I think the risk profile is pretty small. The release notes are updated once per release; it's not like a "normal" documentation page where links could be updated in any pull request (or an external documentation source could change). I think I can live with the ignore, since it will speed things up for most docs builds, and as releases build up, the list of links is going to get absurd - to the point where even N-fold parallelisation won't help (especially when you consider that we get a handful of links added every Monday because of dependabot).
.. _AppImage: ./linux/appimage.html | ||
|
||
.. |Flatpak| replace:: **Flatpak** | ||
.. _Flatpak: ./linux/flatpak.html |
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.
So - the reason these replace calls exist at all is so that we can make the headings bold inside a link. The Bold was intended to indicate the platform default. As long as you don't need the bold, you don't strictly need the replace, either.
I've tweaked to retain the replace, but remove the bolds.
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.
Ahhh, I see; the bold represents defaults. That apparently never clicked with me. I thought the differences in appearance in the table were being driving by the differences in how the links were constructed....and I rationalized that the link constructions were different because of some sphinx/rest thing I didn't understand....
Well, I may not have changed anything if I knew that before 😆
Changes
tox -e docs-lint
spends most of its time now checking links to the Briefcase repo.PR Checklist: