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

Changing docs for trivial issues #50

Open
slateny opened this issue Apr 30, 2022 · 28 comments
Open

Changing docs for trivial issues #50

slateny opened this issue Apr 30, 2022 · 28 comments
Labels
doc:styleguide Future styleguide content

Comments

@slateny
Copy link

slateny commented Apr 30, 2022

There's this open issue right now on British vs American spelling:

python/cpython#82221

In the past, there's been some merged PRs on similar smallish consistency/grammar issues:

python/cpython#2787
python/cpython#25985

However, there are also these other comments that seem to resist against trivial changes:

python/cpython#58372 (comment)
python/cpython#62480 (comment)

The sense that I'm getting is that trivial / minor grammatical / consistency issues are generally not considered if it's either unreported, too large such that it'll take more core dev reviewing time than it's worth in improvement, or touches on somewhat historical docs. Would this be an accurate summary on whether to consider committing small changes?

@CAM-Gerlach
Copy link
Member

(Minor sidenote: Just FYI, to make it easier for others to review the cases you mentioned, can you make them actual inline Markdown links, or at least just inline text or in a bulletted list, which would get linked automatically, instead of code blocks? And to note, you can automatically create inline links to other GitHub issues with helpful popup summaries just by writing /#NNNN , e.g. python/cpython#82221 , python/cpython#2787 python/cpython#25985 , etc.)

@slateny
Copy link
Author

slateny commented Apr 30, 2022

Yep, I can swap them no problem, I just didn't want auto references in the linked PRs/issues since I felt that these aren't directly related to the issues/PRs, and I'm just grabbing them as an example for the question I had. But cross referencing might be useful actually if someone comes along with the same questions as me

@CAM-Gerlach
Copy link
Member

I see; I thought it wouldn't cross reference if you provide a full link, but maybe you know better than I, heh.

@JelleZijlstra
Copy link
Member

I think small improvements are OK, but they do really need to be improvements. If it's unclear whether the change really improves things, I'd rather stick to the status quo.

My thoughts on your examples:

  • writeable vs writable: the conclusion was that both are valid spellings, so we should just stick with the one we already use
  • color vs colour on IDLE: I think we should follow Wikipedia's rule (https://en.wikipedia.org/wiki/Wikipedia:Manual_of_Style#National_varieties_of_English), which is that the original author of a document gets to pick the variety of English used, and then we stick with that. So I would not have approved a wholesale change of "colour" to "color".
  • uppercase vs upper-case: didn't look too closely but seems fine to improve the spelling

@willingc
Copy link
Collaborator

willingc commented May 2, 2022

I agree with @JelleZijlstra's approach re: the examples.

It would probably make sense to update the devguide's docs sections or use that to create a docs style guide. It would be helpful so that people would know the preferred style.

@CAM-Gerlach
Copy link
Member

color vs colour on IDLE: I think we should follow Wikipedia's rule, which is that the original author of a document gets to pick the variety of English used, and then we stick with that. So I would not have approved a wholesale change of "colour" to "color".

This is the rule we generally follow on the PEPs repo as well, since we have both AmE and BrE natives, along with avoiding verbiage specific to one or the other that could potentially impede clear understanding by all.

It would probably make sense to update the devguide's docs sections or use that to create a docs style guide. It would be helpful so that people would know the preferred style.

So long as there's some consensus on what the style should be (or at least that should be one), I'd be happy to help with such effort, given I have some experience with various docs style guides, and writing our own detailed one for the Spyder documentation projects.

@willingc
Copy link
Collaborator

willingc commented May 2, 2022

Hi @slateny, Thank you for your interest and effort in contributing to CPython and its docs. I appreciate your effort. Would you like triage privileges on this repo?

@slateny
Copy link
Author

slateny commented May 2, 2022

Hi @slateny, Thank you for your interest and effort in contributing to CPython and its docs. I appreciate your effort. Would you like triage privileges on this repo?

Thanks Carol, Irit's actually opened a request for me here already :)

@CAM-Gerlach
Copy link
Member

I'm not sure whether or not becoming a CPython triager grants those same permissions on this repo? I think I'd have to be an admin to check...

@AlexWaygood
Copy link
Member

I'm not sure whether or not becoming a CPython triager grants those same permissions on this repo? I think I'd have to be an admin to check...

I'm a CPython triager and I appear to have no special privileges on this repo, so, seems like it doesn't :)

@willingc
Copy link
Collaborator

willingc commented May 3, 2022

@AlexWaygood I just added all of the cpython triagers to this repo. Could you please see if you now have triage privileges? Thanks.

@willingc
Copy link
Collaborator

willingc commented May 3, 2022

@CAM-Gerlach added the CPython triagers to this repo too.

@AlexWaygood
Copy link
Member

@AlexWaygood I just added all of the cpython triagers to this repo. Could you please see if you now have triage privileges? Thanks.

I do! How exciting :D

@willingc willingc added the doc:styleguide Future styleguide content label May 3, 2022
@slateny
Copy link
Author

slateny commented May 19, 2022

Would it be fair to say that these are generally true?

  • if there's two near identical spellings / grammar quirks (hyphenation, AmE/BrE), then the current majority in the article should be applied, otherwise if there's a rough split then the older spelling holds. However, there doesn't need to be consistency between different articles
  • changes that don't greatly affect readability (maybe wording changes like, 'a file that's not standard' vs 'a non-standard file') are unneeded unless to fix consistency as above

I think maybe some justification on why certain rules are in place would be nice as well, if a style guide is made, something like 'spelling consistency within an article is encouraged as it's easier to do a keyword search on a page'

@CAM-Gerlach
Copy link
Member

changes that don't greatly affect readability (maybe wording changes like, 'a file that's not standard' vs 'a non-standard file') are unneeded unless to fix consistency as above

Maybe I'm being a little too nitpicky with your example, but it would really depend on the context and if something was already being changed on that line or nearby. To me, there's a potential distinction in implied meaning that could affect readability and interpretation, depending on the context.

More broadly, I wouldn't expect someone to make such a change unless there was some motivation, and each instance can be evaluated on its own merits with the benefit of context, rather than trying to make detailed, prescriptive rules to cover every possible case and exceptions thereof. Rather, any rule should focus more on the high-level bottom-line—does this change improve the overall quality (clarity, readability, correctness and comprehensiveness) of the documentation?

@ezio-melotti
Copy link
Member

ezio-melotti commented Jun 11, 2022

My two (three?) cents on this:

  • If something is wrong, it should be fixed.
    • This includes typos, should of instead of should have, e.g. instead of i.e. (and vice versa), and similar issues -- even if they are one-character fixes.
    • In general, the fix should also be backported to all applicable branches1.
    • The fix could also span multiple documents if a similar error has been spot elsewhere.
  • If something is inconsistent or ambiguous2, it could be changed.
    • This includes things like AE vs BE, title-case vs sentence-case, rewording a single sentence, etc..
    • In general, the change could be backported1.
    • The change should be limited to a single document (or a single package).
    • If the change adds too much churn (e.g. rewrapping long paragraphs, adding double spaces at the end of each sentence), it can be rejected3.
  • If something is not clearly wrong or inconsistent, it should be left alone.
    • This include things like hyphenations or certain style changes (e.g. adding Oxford commas).

A few more things to keep in mind:

  • Users that create trivial PRs with the sole goal of becoming contributors and/or gaining badges, should be discouraged to do so by rejecting their PRs and/or by explaining them why. This should also be documented so that we could point them to the docs.
  • Valid PRs (even if trivial), shouldn't be closed/rejected or not backported just because they waste developers' time -- some devs don't mind a bit of simple busywork here and there.

Footnotes

  1. There are two arguments in favor of this: people often read old docs online and it makes other backports easier. 2

  2. See my comment below

  3. This kind of changes interfere with git blame, however they can be hidden.

@AA-Turner
Copy link
Member

Valid PRs (even if trivial), shouldn't be closed/rejected or not backported just because they waste developers' time

I think it was a comment that Ned made that the simple fixes for documentation are worth doing as they affect the millions who read the docs, rather than the few hundred people who regularly read Python's source code. (I agree wholeheartedly.)

A

@CAM-Gerlach
Copy link
Member

I fully agree with @ezio-melotti 's comprehensive and well-articulated reasoning above, FWIW.

I think it was a comment that Ned made that the simple fixes for documentation are worth doing as they affect the millions who read the docs, rather than the few hundred people who regularly read Python's source code. (I agree wholeheartedly.)

Yes, indeed, though one critical distinction here (and one I haven't always been diligent about in the past) to keep in mind is changes that just affect the style/formatting of the source code, which is only seen by a few hundred people, versus those that meaningfully impact the final output—and further, for source-code only modifications, both changes and strategies/patterns of changes that increase vs. decrease the cost of present and future maintenance and improvements.

@mwichmann
Copy link

mwichmann commented Jun 12, 2022

Regarding this:

If the change adds too much churn (e.g. rewrapping long paragraphs, adding double spaces at the end of each sentence), it can be rejected[2]
2. This kind of changes interfere with git blame, however they can be hidden

They still affect other ways people browse history, like git diff. Further, if you do this, the reformatting-to-be-ignored should be in a change which only reformats, and does not also add other changes, or ignoring the revision is a bad thing to do. I'd be cautious with these - one bulk reformat (like people do on the code parts by running a formatter on the whole codebase) can usually be tolerated, but otherwise it does get rather messy - so leaving a maintainer the ability to reject-because-of-churn-without-getting-unreasonably-flamed indeed seems a good thing to account for.

@AA-Turner
Copy link
Member

so leaving a maintainer the ability to reject-because-of-churn-without-getting-unreasonably-flamed indeed seems a good thing to account for

I might be misunderstanding you & Cam, but I'm not sure I fully see this argument. The documentation is fundamentally for the end user or programmer who reads the rendered HTML rather than the reST sources. Whilst making developers go through an extra step in git for tracking when changes were introduced isn't ideal, I think it is still worth it to fix the documentation in most cases.

So a legitimate reason to close trivial PRs occasionally perhaps, but I would hope it isn't used as a crutch for rejecting change.

A

@merwok
Copy link
Member

merwok commented Jun 12, 2022

Mostly agree with Ezio, except that I’d make a difference between wrong or ambiguous (which should get fixed, with backports) and inconsistent (which is less strong and should be judged case by case).

@mwichmann
Copy link

so leaving a maintainer the ability to reject-because-of-churn-without-getting-unreasonably-flamed indeed seems a good thing to account for

I might be misunderstanding you & Cam, but I'm not sure I fully see this argument. The documentation is fundamentally for the end user or programmer who reads the rendered HTML rather than the reST sources. Whilst making developers go through an extra step in git for tracking when changes were introduced isn't ideal, I think it is still worth it to fix the documentation in most cases.

I don't think there's a disagreement here: a change which reformats the layout of the doc source doesn't provide any visible benefit to the person reading the rendered output, and might make life harder for a future dev trying to sort out when changes were made. Yes I know there have been other threads on whether or not to reflow the rst sources on changes, but just want to keep in mind that maintainer time to merge PRs is a valuable and limited resource.

@ezio-melotti
Copy link
Member

Mostly agree with Ezio, except that I’d make a difference between wrong or ambiguous

To clarify, "ambiguous" was referring to the rules to be applied, not to the meaning of the sentence. For example the meaning of "This tool has a command line interface." is clear, but whether "command line" should be hyphenated or not is ambiguous (I thought it should, but Wiktionary seems to allow both forms).

@CAM-Gerlach
Copy link
Member

I might be misunderstanding you & Cam, but I'm not sure I fully see this argument. The documentation is fundamentally for the end user or programmer who reads the rendered HTML rather than the reST sources. Whilst making developers go through an extra step in git for tracking when changes were introduced isn't ideal, I think it is still worth it to fix the documentation in most cases.

@mwichmann said it perfectly, much better than I did—the contrast here is between changes that simply reformat the reST source (e.g. trailing white space, spaces between sentence, line breaks, etc) versus those that make a meaningful difference to the rendered HTML (e.g. better organization, clearer phrasing, avoiding ambiguity, etc). This is also a discussion we've had before among ourselves on the PEPs repo when it comes to issues of arbitrary conventions versus things that impair understanding (e.g. AmE vs BrE spellings, versus avoiding words specific to one of the dialects that could potentially confuse the other; and respecting the author's convention in existing code where there wasn't a ),

This leads right into a related item worth mentioning here, which this discussion helped motivate my ideas for: the Discourse thread on preferring a more structured format for sections of the stdlib docs that are essentially the API reference. In that case, making it easier for users to quickly find the key information they need would appear to have a clear benefit to readers, even if the individual changes are fairly straightforward and mechanical, and require touching a fair number of lines.

For example the meaning of "This tool has a command line interface." is clear, but whether "command line" should be hyphenated or not is ambiguous

Yeah, In this case, it would certainly be ideal if we could be at least locally consistent with one or the other, something so small is IMO not worth simply for consistency's sake, never mind preferring one or the other, when something like that makes little to no practical difference to the reader (if the notice at all).

@encukou
Copy link
Member

encukou commented Jul 18, 2022

I see people agreeing with each other :)
Is it time for a pull request for the style guide?

@CAM-Gerlach
Copy link
Member

Maybe, but at least for myself I'm not 100% sure how (or perhaps even whether) we should actually codify our consensus, since this is more of a high-level conceptual issue than the concrete guidance I'm used to dealing with in style guides.

@encukou
Copy link
Member

encukou commented Aug 4, 2022

IMO the style guide is the best place we have for this, and it's a part of the devguide where this kind of info fits even better.

@CAM-Gerlach
Copy link
Member

Ah, looking more carefully, it seems devguide's "style guide" is devoted primarily to high-level conceptual issues like this, while the "Markup Guide" deals with the nitty gritty that I'm more used to seeing in style guides.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc:styleguide Future styleguide content
Projects
None yet
Development

No branches or pull requests

10 participants