-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
Use hyphenated version directives #13714
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
base: master
Are you sure you want to change the base?
Use hyphenated version directives #13714
Conversation
Is this really worth it? Yes, consistency is nice, but I’ve used all these directives and I’ve never stumbled on the naming; If this is added, I’d at least not deprecate |
Is it needed? No. But it's helpful to have the set of
I agree. That's my intention in only noting it in the docs: I specifically did not want to create busy work for people. |
I'd be happy to add alternative forms with a hyphen, we've done this already for various directive options ( A |
ff4146d
to
6e01dbf
Compare
Sure. Done. |
IMHO the hyphened version is a readability improvement. OTOH it is a massive change. I'm +0.5 overall., but I leave it to @AA-Turner to do the tradeoff. Concerning the PR: I suggest to split the actual change documentation and announcement from the mass-update. - At least by commits, but possibly even into separate PRs. First lets get the documentation and communication right - this is a small change but needs care to detail, because this change eventually affects a significant part of our users. Then, let's do our internal cleanup, which is a lot of churn but mostly a trivial replacement. |
I think the fact that we are not deprecating (in code) - and IMO should never deprecate - the older mechanism would limit the fallout of this. Newer docs can benefit from improved readability but older docs continue to work.
I've mostly done this already, but I can remove the outstanding changes to |
I've stripped this PR back to only introduce the new hypenated versions. |
47d33d9
to
fa2b30f
Compare
Please don't force-push, it makes reviews harder on GH. Please could you also go back to the previous state of combined directives for both versions, rather than separate 'legacy alias' blocks? If we merge the hyphenated forms, I don't want to 'officially' deprecate the current forms for a year or two. A |
Use e.g. 'version-changed' over 'versionchanged'. While the deprecated admonition is marked as deprecated itself in the docs, we do not do anything at runtime since there is almost zero cost to keeping this relative to the work needed for millions of documents to be updated. While here, we also update a use of the deprecated admonition with version-changed since it makes more sense in the context (an attribute of the feature was deprecated but not the whole feature itself). Signed-off-by: Stephen Finucane <[email protected]>
7986264
to
8d3d608
Compare
Okay. I had to force push a final time to correct errant info in the commit message (I was still referencing the rework of the docs, which we're no longer doing here). If no force pushes is a requirements, could I suggest adding this to the contributor guide (linked from
Done, I think (assuming I understood the ask correctly). I've said this before though and I'll say it again: I really hope we don't ever remove the current forms. Reworking an existing project would be pure busywork for the same of 8 lines of code in Sphinx. If that's going to be a necessity I'd rather abandon this PR, tbh. |
Good idea. It's not a hard requirement -- force pushing is generally fine on a 'fresh' PR, but limitations in GitHub's review interface make it annoying for PRs that have been reviewed etc (e.g. the 'show recent changes' button no longer works, previous unresolved review comments are lost).
We've used squash-merge in Sphinx since c. January 2023, which I find keeps the git history much cleaner. It's the same approach that we use in CPython and the other @python repositories with some sucecss. It means that the PR branch can be 'messy' but keeps the final commit as a single unit. But this is a somewhat recent change in workflow, so I agree good to write it down somewhere.
Sorry, I meant the state at sphinx/doc/usage/restructuredtext/directives.rst Lines 540 to 541 in 13796db
Whilst we're on the topic, what are your thoughts on A |
Understood. Let me put something together later on today. (As an aside, it is mighty unfortunate that the GitHub PR/GitLab MR model is what caught on, as opposed to Gerrit's commit == change model. Alas)
Gotcha. I'll push that shortly.
I would personally be inclined not to remove this. By all means, we could stop documenting it, but when you consider the many thousands (if not millions) of documents that would need changing and the resulting hours that would be wasted to allow us to delete a handful of lines here, it seems wrong. Spoken as someone who still has scars from setuptool's decision to remove support for hyphen-separated options for the otherwise identical underscore-separated options 😅 |
We also remove an errant 'version-added' that crept in in a previous patch. Signed-off-by: Stephen Finucane <[email protected]>
Purpose
We alias the versioning directives like so:
version-added
(wasversionadded
)version-changed
(wasversionchanged
)version-removed
(wasversionremoved
)version-deprecated
(wasdeprecated
)The original names are marked as deprecated in the docs but do no produce build-time warnings, since we don't want to break 1000s of builds using
-W
(warning is error) over this.(edit: updated since creation to reflect changes in PR)
References
(none)