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

gh-99308: Clarify re docs for byte pattern group names #99311

Merged
merged 3 commits into from Dec 25, 2022

Conversation

Kentzo
Copy link
Contributor

@Kentzo Kentzo commented Nov 10, 2022

@@ -496,6 +496,8 @@ The special characters are:

.. versionchanged:: 3.12
Group *id* can only contain ASCII digits.
In bytes replacement strings group names must contain only characters
Copy link
Member

@merwok merwok Nov 10, 2022

Choose a reason for hiding this comment

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

Let’s avoid five nouns in a row 😄

Suggested change
In bytes replacement strings group names must contain only characters
In bytes replacement strings, group names must contain only characters

In bytes replacement strings group names must contain only characters
in the ASCII range.
Copy link
Member

@CAM-Gerlach CAM-Gerlach Nov 10, 2022

Choose a reason for hiding this comment

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

Maybe its just me, but this was pretty confusing to me and hard to parse and understand. The comma @merwok suggest helps a lot in making the basic grammar more clear, but there still seems to be some problems with the actual semantics here.

As far as I can tell, this should refer to patterns, not replacements, and I was very confused why the latter were mentioned until I realized it was incorrectly copy/pasted from the original mention (that talks about replacement strings).

Also, "bytes strings" is both arguably an oxymoron in Python 3 (since this is presumably intended to specifically refer to bytes, not strings), seemingly ungrammatical (at least without being proper type references), and certainly confusing.

Finally, name should be formatted consistently with id in the previous line, :class:bytes objects are composed of bytes, not characters (though perhaps that's being too pedantic), and it would be useful to explictly specify what the ASCII range is. Implementing all of that, and fixing the phrasing, we have:

Suggested change
In bytes replacement strings group names must contain only characters
in the ASCII range.
In :class:`bytes` patterns, group *name* can only contain bytes
in the ASCII range (``b'\x00'``-``b'\x7f'``).

Or, with decimal values for the ASCII range,

Suggested change
In bytes replacement strings group names must contain only characters
in the ASCII range.
In :class:`bytes` patterns, group *name* can only contain bytes
in the ASCII range (values 0-127).

Copy link
Contributor Author

@Kentzo Kentzo Nov 14, 2022

Choose a reason for hiding this comment

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

@CAM-Gerlach Maybe b'\x00'-b'\x7f'?

Copy link
Member

@CAM-Gerlach CAM-Gerlach Nov 14, 2022

Choose a reason for hiding this comment

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

I guess so; edited. I just didn't want to overcomplicate it.

Copy link
Member

@CAM-Gerlach CAM-Gerlach left a comment

Seems LGTM now from my side, thanks. 👍

@CAM-Gerlach CAM-Gerlach requested a review from merwok Dec 1, 2022
@hauntsaninja hauntsaninja changed the title gh-99308: Fix re docstring gh-99308: Clarify re docs for byte pattern group names Dec 22, 2022
Copy link
Contributor

@hauntsaninja hauntsaninja left a comment

Thanks for improving this!

This looks good to me, although my ignorable nit is that "ASCII range" alone seems sufficient to me: specifying the hex values doesn't add much since group names need to be valid Python identifiers.

@kumaraditya303 kumaraditya303 merged commit dbc1e69 into python:main Dec 25, 2022
14 checks passed
@miss-islington
Copy link
Contributor

miss-islington commented Dec 25, 2022

Thanks @Kentzo for the PR, and @kumaraditya303 for merging it 🌮🎉.. I'm working now to backport this PR to: 3.11.
🐍🍒🤖

@miss-islington
Copy link
Contributor

miss-islington commented Dec 25, 2022

Sorry, @Kentzo and @kumaraditya303, I could not cleanly backport this to 3.11 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker dbc1e696ebf273bc62545d999eb185d6c9470e71 3.11

@kumaraditya303
Copy link
Contributor

kumaraditya303 commented Dec 25, 2022

Skipping backport as it is for 3.12 only.

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 news
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants