Skip to content

Misc copyedits in docs on built-in types#24466

Merged
miss-islington merged 16 commits intopython:mainfrom
adorilson:library-stdtypes
Nov 17, 2022
Merged

Misc copyedits in docs on built-in types#24466
miss-islington merged 16 commits intopython:mainfrom
adorilson:library-stdtypes

Conversation

@adorilson
Copy link
Contributor

@adorilson adorilson commented Feb 7, 2021

DOC: Improvements in library/stdtypes

This PR does the following:

  1. Replaces :meth: by :func: around repr function
  2. Adds links to Unicode Standard site
  3. Makes explicit "when" you can call the iskeyword function. The previous text could cause confusion to readers, especially those with English as a second language. The reader could understand that the isidentifier method calls the iskeyword function. Now, it is explicit that the dev can do it.
  4. Replaces a URL with an inline link.

Automerge-Triggered-By: GH:AlexWaygood

The previous text can make a confusion in reader. The reader
could understand the isidentifier method call the iskeyword.
Now, is explicit that the dev can do it.
@github-actions
Copy link

This PR is stale because it has been open for 30 days with no activity.

@github-actions github-actions bot added the stale Stale PR or inactive for long period of time. label Mar 10, 2021
@MaxwellDupre
Copy link
Contributor

Trying to compile I get:
make html
mkdir -p build
Building NEWS from Misc/NEWS.d with blurb
PATH=./venv/bin:$PATH sphinx-build -b html -d build/doctrees -W . build/html
Running Sphinx v4.5.0
loading pickled environment... done
building [mo]: targets for 0 po files that are out of date
building [html]: targets for 496 source files that are out of date
updating environment: [config changed ('version')] 485 added, 0 changed, 14 removed
reading sources... [100%] whatsnew/index

Warning, treated as error:
/home/me/Documents/cpython/Doc/c-api/complex.rst:49:Error in declarator or parameters
Error in declarator or parameters
Invalid C declaration: Expected identifier, got user-defined keyword: complex. Remove it from c_extra_keywords to allow it as identifier.
Currently c_extra_keywords is ['alignas', 'alignof', 'bool', 'complex', 'imaginary', 'noreturn', 'static_assert', 'thread_local']. [error at 39]
Py_complex _Py_c_neg(Py_complex complex)
---------------------------------------^
make: *** [Makefile:51: build] Error 2

cpython/Doc on  pr/24466 [$] via 🐍 v3.11.0a6+ took 1m16s


My environment should be ok. I did have an issue but this was made good yesterday and I am using the recommended Sphinx version, also has been compiling the Docs ok up to now.
I know this shows the error is with complex.rst but cant confirm all good. Plus noted you have conflicts with your branch.

Comment on lines 1794 to 1796
Copy link
Member

@AlexWaygood AlexWaygood May 17, 2022

Choose a reason for hiding this comment

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

-1 on this change. This makes it more verbose but doesn't significantly improve clarity, in my opinion. Instead, why not simply italicise the word "reserved"?

The other changes LGTM

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The change made here was Call --> You can call.
The current text can to let the reader to understand which is the isidentifier internally call the iskeyword method (mainly is is not a native speaker, I guess). But it is not true. So, the propose is to make explicit that the programmer must to call the function.

In the reserved was just a break line. No changed the content.

Copy link
Member

Choose a reason for hiding this comment

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

The change made here was Call --> You can call.

I know, but, in my opinion, the imperative mood works fine here :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The change made here was Call --> You can call.

I know, but, in my opinion, the imperative mood works fine here :)

I have experiences where the imperative was not fine. :)

Anyway, you are free to revert the commit (I guess). Help yourself.

Or will ask for a third opinion.

Copy link
Member

Choose a reason for hiding this comment

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

I'm just a triager, so I don't have the power to do anything! Just giving my opinion :)

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree that imperative seems fine, but can you share the experiences where you found that imperative caused problems? I'd be interested in hearing the examples and seeing if they'd apply here.

Copy link
Member

@CAM-Gerlach CAM-Gerlach Oct 16, 2022

Choose a reason for hiding this comment

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

As a technical docs writer and copyeditor, imperative is generally fairly idiomatic in reference-type documentation such as this, and initially I didn't see a problem with it either.

However, looking more carefully at the context, I do see an issue with it: the imperative mood is conventionally used, as it is on the line right before, to describe what the method does. However, this next sentence instead describes what the user might want to consider doing, and the chosen phrasing, unlike the standard imperative, makes explicit that distinction that might otherwise cause confusion, especially among readers with less native English proficiency.

Of course, such asides with suggestions for the reader aren't always appropriate in reference-type documentation, especially using language like "You can", but with a tiny bit of linguistic massaging it can fit right in (unfortunately, it won't let me actually suggest here):

   :func:`keyword.iskeyword` can be used to test whether string ``s`` is a
   reserved identifier, such as :keyword:`def` and :keyword:`class`.

I have experiences where the imperative was not fine. :)

I would also appreciate clarification here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As a technical docs writer and copyeditor, imperative is generally fairly idiomatic in reference-type documentation such as this, and initially I didn't see a problem with it either.

However, looking more carefully at the context, I do see an issue with it: the imperative mood is conventionally used, as it is on the line right before, to describe what the method does. However, this next sentence instead describes what the user might want to consider doing, and the chosen phrasing, unlike the standard imperative, makes explicit that distinction that might otherwise cause confusion, especially among readers with less native English proficiency.

Exactly this. Especially if the native language be Portuguese.

Of course, such asides with suggestions for the reader aren't always appropriate in reference-type documentation, especially using language like "You can", but with a tiny bit of linguistic massaging it can fit right in (unfortunately, it won't let me actually suggest here):

   :func:`keyword.iskeyword` can be used to test whether string ``s`` is a
   reserved identifier, such as :keyword:`def` and :keyword:`class`.

I have experiences where the imperative was not fine. :)

I would also appreciate clarification here

One experience mine was: a "readers with less native English proficiency" read the actual text and understood that the method itself call the other method, not that the developer must do the call. So, is this reason of my suggestion.

Copy link
Member

Choose a reason for hiding this comment

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

See suggestion implementing the above

@github-actions github-actions bot removed the stale Stale PR or inactive for long period of time. label Aug 4, 2022
Copy link
Member

@CAM-Gerlach CAM-Gerlach left a comment

Choose a reason for hiding this comment

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

This is a good start; I've suggested a number of refinements.

Comment on lines 1794 to 1796
Copy link
Member

Choose a reason for hiding this comment

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

See suggestion implementing the above

@CAM-Gerlach CAM-Gerlach added needs backport to 3.10 only security fixes needs backport to 3.11 only security fixes labels Nov 13, 2022
@CAM-Gerlach
Copy link
Member

CAM-Gerlach commented Nov 13, 2022

The checks were run long enough ago that I can't see what failed, but merging with main should hopefully fix it. Otherwise, I can give it a closer look.

@CAM-Gerlach
Copy link
Member

Ah, its just failing because of some trailing whitespace, but that's already fixed in my suggestions, so just apply those and you should be good to go.

FYI, in case you aren't aware, you can do so by going to the Files changed tab, click Add to batch on the suggestions you want to apply, then once you've selected them all, click Commit on any suggestion or up top to commit them all in one go directly to the PR and auto-resolve the linked review comments.

adorilson and others added 6 commits November 16, 2022 10:29
Co-authored-by: C.A.M. Gerlach <CAM.Gerlach@Gerlach.CAM>
Co-authored-by: C.A.M. Gerlach <CAM.Gerlach@Gerlach.CAM>
Co-authored-by: C.A.M. Gerlach <CAM.Gerlach@Gerlach.CAM>
Link the actual case conversion subsection of Unicode's section 3.13

Co-authored-by: C.A.M. Gerlach <CAM.Gerlach@Gerlach.CAM>
Co-authored-by: C.A.M. Gerlach <CAM.Gerlach@Gerlach.CAM>
Co-authored-by: C.A.M. Gerlach <CAM.Gerlach@Gerlach.CAM>
@adorilson
Copy link
Contributor Author

Ah, its just failing because of some trailing whitespace, but that's already fixed in my suggestions, so just apply those and you should be good to go.

FYI, in case you aren't aware, you can do so by going to the Files changed tab, click Add to batch on the suggestions you want to apply, then once you've selected them all, click Commit on any suggestion or up top to commit them all in one go directly to the PR and auto-resolve the linked review comments.

In fact, I was not aware about this. However, I read this comment too late. I will use this resource in a next time.

Thanks for suggestions and good discussion.

Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks @adorilson for the PR, and thanks @CAM-Gerlach for the detailed review comments!

@AlexWaygood AlexWaygood self-assigned this Nov 17, 2022
@AlexWaygood AlexWaygood changed the title DOC: Improvements in library/stdtypes Misc copyedits in docs on built-in types Nov 17, 2022
@miss-islington
Copy link
Contributor

Status check is done, and it's a success ✅.

@miss-islington miss-islington merged commit a0d940d into python:main Nov 17, 2022
@miss-islington
Copy link
Contributor

Thanks @adorilson for the PR 🌮🎉.. I'm working now to backport this PR to: 3.10, 3.11.
🐍🍒⛏🤖

@miss-islington
Copy link
Contributor

Sorry, @adorilson, I could not cleanly backport this to 3.11 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker a0d940d6acbb5c6614cf892192d8cb0d7002e5a6 3.11

@miss-islington
Copy link
Contributor

Sorry @adorilson, I had trouble checking out the 3.10 backport branch.
Please retry by removing and re-adding the "needs backport to 3.10" label.
Alternatively, you can backport using cherry_picker on the command line.
cherry_picker a0d940d6acbb5c6614cf892192d8cb0d7002e5a6 3.10

@AlexWaygood
Copy link
Member

AlexWaygood commented Nov 17, 2022

Backporting this is actually slightly complicated, since the 3.11 branch is still using v14 of the Unicode standard, and the links added in this PR all point to v15 of the Unicode standard. I'm not going to undertake backporting this myself, but if you want to do a manual backport to the 3.11 branch @adorilson (without the Unicode-standard-related changes), feel free to ping me, and I can get it merged.

@AlexWaygood AlexWaygood removed needs backport to 3.10 only security fixes needs backport to 3.11 only security fixes labels Nov 17, 2022
@CAM-Gerlach
Copy link
Member

CAM-Gerlach commented Nov 17, 2022

@AlexWaygood When whomever is manually backporting, the links could simply be adjusted to point to Unicode 14.0, or Unicode 13.0 for 3.10.

@AlexWaygood
Copy link
Member

@AlexWaygood When whomever is manually backporting, the links could simply be adjusted to point to Unicode 14.0, or Unicode 13.0 for 3.10.

Indeed they could, but on balance, I'd prefer they weren't. I think backports should generally be as minimal as possible, and should generally differ from the original PR as little as possible. This reduces the risk of discrepancies emerging between the branches, which might complicate future backports.

But, I don't have a particularly strong opinion about it.

@CAM-Gerlach
Copy link
Member

CAM-Gerlach commented Nov 17, 2022

It seems I'm a little confused, sorry—I thought you were suggesting above to not backport the change at all, or to completely omit a substantial part of the changes. However, it now sounds like you're suggesting backporting it as-is without changing the links, which would be the minimal delta to the original PR, and equally the lowest chance of creating merge conflicts with future backports (which is certainly a concern that I share). The next-most-minimal diff to both the original PR and between the branches would be just tweaking only the versions in the URLs, which it seems you're not in favor of, if I understand you right?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

docs Documentation in the Doc dir skip issue skip news

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants

Comments