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

PEP 675: List the str methods that preserve literal types #2282

Merged
merged 9 commits into from
Feb 1, 2022

Conversation

pradeep90
Copy link
Contributor

cc @gbleaney

@JelleZijlstra

Mostly looking for feedback on the two design choices we need to make:

  1. Do we restrict literal-preserving str methods to (a) just the four mentioned in "Type Inference" or (b) to the full set of str methods?
  2. Do we (a) overload the typeshed stubs for those methods or (b) leave the str stub as is and ask type checkers to hardcode literal behavior?

We are leaning towards 1b and 2a. Let us know your thoughts. We have also offered to change it if the Steering Council feels strongly.

Also, I recall that, as a typeshed maintainer, you wanted us to give clear recommendations for future typeshed changes. Could you clarify the questions we need to address?

Finally, we'll change the name from Literal[str] in a PR after the above stuff is resolved.

Copy link
Member

@JelleZijlstra JelleZijlstra left a comment

Choose a reason for hiding this comment

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

I'd prefer 2a. Documenting the decision in typeshed makes it more discoverable than a set-in-stone list in a PEP that every type checker has to copy.

Question 1 is related to my previous query about typeshed. Basically, as a typeshed maintainer I want to know when I should accept a PR that adds Literal[str] to a method. Perhaps we could say something like "In typeshed, any pure function returning a string may/should be annotated as returning Literal[str] when the arguments are literals.", with some suitable definition of "pure".

For example, if someone wants us to add a Literal[str] overload for https://docs.python.org/3/library/fnmatch.html#fnmatch.translate (random example from git grep 'def .*: str.*-> str'), what should we say?

pep-0675.rst Outdated Show resolved Hide resolved
pep-0675.rst Outdated Show resolved Hide resolved
pep-0675.rst Outdated Show resolved Hide resolved
pep-0675.rst Outdated
checkers may need to special-case ``str`` so that error messages are
understandable for users.

If the Steering Council is opposed to this typeshed stub change, we
Copy link
Member

Choose a reason for hiding this comment

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

This feels too low-level for the SC to decide, unless you have concrete indications that it's something they're concerned about. I'd prefer if the PEP takes a concrete stance, otherwise it just becomes something that can drag out the discussion. (I don't want another PEP 646.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd prefer if the PEP takes a concrete stance

Makes sense. I'll add the concrete stance we've agreed on and remove this offer.

(I don't want another PEP 646.)

Too soon, Jelle. Too soon :)

pep-0675.rst Outdated Show resolved Hide resolved
pep-0675.rst Outdated Show resolved Hide resolved
@CAM-Gerlach CAM-Gerlach changed the title PEP 675: List the str methods that preserve literal types. PEP 675: List the str methods that preserve literal types Jan 27, 2022
@CAM-Gerlach
Copy link
Member

Just to note, as others have raised it when I've done the same thing—generally speaking major substantive discussion of PEP contents, particularly high level feedback on design choices, is "supposed" to happen on the PEP's discussion thread, or (if necessary for more granular feedback before being discussed by a wider audience), a PR on the author's fork (or elsewhere). PRs on the main PEPs repo are "supposed" to only be made once the changes are ready for PEP editor review and merging, rather than a place to iterate.

(Also, minor nit, but PR titles, like all titles in English [and Git commit summary messages], don't end with a period)

@JelleZijlstra
Copy link
Member

I think discussion among the PEP authors and sponsors for a pending PR is fine, especially when the author directly asks for feedback.

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.

Fix the syntax error causing a build failure and a few other RST syntax issues

pep-0675.rst Outdated Show resolved Hide resolved
pep-0675.rst Outdated
Comment on lines 545 to 546
As PEP 586 `mentions
<https://www.python.org/dev/peps/pep-0586/#backwards-compatibility>`_,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
As PEP 586 `mentions
<https://www.python.org/dev/peps/pep-0586/#backwards-compatibility>`_,
As PEP 586 :pep:`mentions <586#backwards-compatibility>`,

Is there a reason this was reverted? It is simpler and more reliable to use :pep: to refer to PEPs rather than hardcoding the full absolute URL, and will allow us to do things like show the PEP's title in the titletext and other things in the future.

Copy link
Member

Choose a reason for hiding this comment

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

@CAM-Gerlach can we add a lint check for this? It could probably look for python.org/dev/peps with a whitelist of ones we don't want to change [yet] (a few in 1,12,287 from memory).

A

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there a reason this was reverted? It is simpler and more reliable to use :pep: to refer to PEPs rather than hardcoding the full absolute URL, and will allow us to do things like show the PEP's title in the titletext and other things in the future

Huh. The official PEP page renders the :pep: link correctly but the GitHub "View file" Markdown preview doesn't. I was going by the latter. Will revert back to the :pep: version.

Copy link
Member

Choose a reason for hiding this comment

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

can we add a lint check for this? It could probably look for python.org/dev/peps with a whitelist of ones we don't want to change [yet] (a few in 1,12,287 from memory).

Good idea; in work. If/when PEP 676 is accepted and we transition to the Sphinx-based infra, we could also add one for hardcoded links to PEP supplementary files, instead of using :download: (or another appropriate role)—unless you have a good way to do this now that works across both the legacy and new build systems and doesn't break when changing the output mode (files/directories/etc).

Huh. The official PEP page renders the :pep: link correctly but the GitHub "View file" Markdown preview doesn't. I was going by the latter. Will revert back to the :pep: version.

The GitHub Markdown preview only has very limited compatibility with RST syntax and should never be relied upon; we will have rendered previews on PRs soon assuming PEP 676 is accepted (or possibly before, depending on when @AA-Turner adds it). Until then, please follow the instructions in the Readme/Contributing guide (basically, install the requirements.txt then run the build.py script for the new build system or the pep2html.py one for the old one to build a preview.

pep-0675.rst Outdated Show resolved Hide resolved
pep-0675.rst Outdated Show resolved Hide resolved
pep-0675.rst Outdated Show resolved Hide resolved
@CAM-Gerlach
Copy link
Member

Hey @pradeep90 I see all the suggestions were marked as resolved, but they apparently didn't get applied—any idea what's up?

@pradeep90
Copy link
Contributor Author

@CAM-Gerlach I'm working through them and marking them as resolved. Will push all my commits and request re-review when I'm done

@CAM-Gerlach
Copy link
Member

FYI, if you go to the files tab of the PR, you can click "Add to batch" on each suggestion and then use the "Commit" button at the top to commit them, preferably with an informative message. This commits the suggestions directly to your branch (which you then can pull, etc. locally) in one click, marks and auto-resolves the linked review comments, and gives the suggestion contributor attribution (which isn't a bit deal to me, but matters to some people). This is much easier and faster than applying them manually, avoiding extra work for you and the chance of making a mistake or missing a suggestion, and is also easier for reviewers, as its clear what suggestions were applied and in what commits.

Copy link
Member

@JelleZijlstra JelleZijlstra left a comment

Choose a reason for hiding this comment

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

Thanks! Noticed one typo, I'll push the fix

pep-0675.rst Outdated Show resolved Hide resolved
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.

LGTM, thanks @pradeep90!

@pradeep90
Copy link
Contributor Author

@JelleZijlstra I did a full pass of the PEP and fixed a bunch of issues in the latest commit.

@pradeep90
Copy link
Contributor Author

@JelleZijlstra gentle reminder in case this slipped off your radar.

(Working on the name change PR now.)

Comment on lines +969 to +973
# after
@overload
def capitalize(self: Literal[str]) -> Literal[str]: ...
@overload
def capitalize(self) -> str: ...
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't know if that's the right place for it to ask.

Instead of overloads, wouldn't it also work / be better to use a bound TypeVar or the new Self type? The way I understand the PEP, LiteralString should be defined as subtype of str within typeshed.

From the PEP:

Rationale

[...] LiteralString is the "supertype" of all literal string types.
In effect, this PEP just introduces a type in the type hierarchy
between Literal["foo"] and str. [...]

Copy link
Member

Choose a reason for hiding this comment

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

A TypeVar wouldn't work for this case, sadly, as the relationship here doesn't generalise to other subtypes of str:

>>> class Foo(str): ...
... 
>>> Foo('hi')
'hi'
>>> type(_)
<class '__main__.Foo'>
>>> Foo('hi').capitalize()
'Hi'
>>> type(_)
<class 'str'>

Copy link
Member

Choose a reason for hiding this comment

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

I think a TypeVar with constraints (similar to AnyStr) might work?

Copy link
Member

Choose a reason for hiding this comment

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

Hmm — that might work, but might also run into mypy bugs similar to python/typeshed#6762?

Copy link
Member

Choose a reason for hiding this comment

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

Don't know if that's the right place for it to ask.

@cdce8p You could also bring it up on the official typing-sig thread for this PEP, which appears to be this one—that might be more generally visible than a comment on a merged PR, if you're looking for a wider discussion.

On a more editorial note, it looks like the PEP hasn't yet been updated with the link to the actual discussion thread, which I only found after a fair amount of determined digging (and guessing). I'll open a PR to add that.

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

Successfully merging this pull request may close these issues.

7 participants