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

bpo-31369: include RegexFlag in re.__all__ #30279

Merged
merged 8 commits into from Feb 5, 2022

Conversation

akulakov
Copy link
Contributor

@akulakov akulakov commented Dec 27, 2021

Doc/library/re.rst Outdated Show resolved Hide resolved
@@ -710,6 +713,9 @@ form.
string and immediately before the newline (if any) at the end of the string.
Corresponds to the inline flag ``(?m)``.

.. data:: NOFLAG

Indicates no flag being applied, the value is ``0``.
Copy link
Member

Choose a reason for hiding this comment

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

Similarly, explain when this is useful and add versionadded.

Copy link
Contributor Author

@akulakov akulakov Jan 20, 2022

Choose a reason for hiding this comment

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

It can be used for example as a default value for a function parameter, and also as a base value that will later conditionally be ORed with other flags. But this explanation would be confusing to many readers without code examples.

An example as follows may be enough:

def myfunc(text, flag=re.NOFLAG):
    return re.match(text, flag)

What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

Thanks! I think that's a useful example.

Copy link
Member

@merwok merwok Feb 10, 2023

Choose a reason for hiding this comment

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

When was it added? Edit: found #72269 this added the enum, this PR here actually added NOFLAG despite its title

It seems that the discussion in #57594 was ignored, plus the naming is slightly stange (no flags reads better than no flag)

Copy link
Member

Choose a reason for hiding this comment

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

Not ignored, missed. It took place 11 years prior. We can certainly add a NOFLAGS name now, and make NOFLAG be an alias.

Lib/re.py Outdated
@@ -137,14 +137,15 @@
"findall", "finditer", "compile", "purge", "template", "escape",
"error", "Pattern", "Match", "A", "I", "L", "M", "S", "X", "U",
"ASCII", "IGNORECASE", "LOCALE", "MULTILINE", "DOTALL", "VERBOSE",
"UNICODE",
"UNICODE", "RegexFlag",
Copy link
Member

Choose a reason for hiding this comment

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

Should also add NOFLAG.

@JelleZijlstra
Copy link
Member

@ethanfurman would you like to look at this as it's enum-related?

@bdice
Copy link

bdice commented Feb 4, 2022

Thanks! I was just looking into this and was puzzled why a NOFLAG (or similar) object didn't exist. Happy to see this PR. 😄

On a related note, I noticed that the type of re.compile("").flags is int and not re.RegexFlag as I expected. I might file an issue for this.

@@ -637,6 +637,11 @@ form.
programs that use only a few regular expressions at a time needn't worry
about compiling regular expressions.

.. class:: RegexFlag

An :class:`~enum.Enum` class containing the regex options listed below.
Copy link
Member

Choose a reason for hiding this comment

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

What does the tilde in ~enum.Enum mean?

Also, it should be enum.IntFlag.

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 tilde creates a link with only the last element visible, i.e. Enum here, but since it should be IntFlag, I also removed the tilde as IntFlag is less known (though reader can still hover over shortened name to see where it links of course).


An :class:`~enum.Enum` class containing the regex options listed below.

.. versionadded:: 3.6 (documented in 3.11)
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps .. versionchanged:: added to all in 3.11

@bedevere-bot
Copy link

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.


.. versionadded:: 3.6 (documented in 3.11)
.. versionadded:: added to ``__all__`` in 3.11)
Copy link
Member

Choose a reason for hiding this comment

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

Missing a parenthesis, so lets change to:

.. versionadded:: 3.11 - added to ``__all__``

@clinton21obeng
Copy link

clinton21obeng commented Feb 5, 2022 via email

@akulakov
Copy link
Contributor Author

akulakov commented Feb 5, 2022

@ethanfurman thanks for reviewing!

@akulakov akulakov deleted the 31369-Add-RegexFlag-to-__all__ branch February 5, 2022 03:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants