Skip to content
This repository has been archived by the owner on Dec 11, 2023. It is now read-only.

Reorganize the spec #16

Merged
merged 31 commits into from Nov 28, 2023
Merged

Reorganize the spec #16

merged 31 commits into from Nov 28, 2023

Conversation

JelleZijlstra
Copy link
Owner

Copy link
Contributor

@Daverball Daverball left a comment

Choose a reason for hiding this comment

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

While this isn't really the point of this PR, it just so happens that it covers the entire text of the spec, so it's easy to make suggestions for some of the low hanging fruits in the examples.

spec/aliases.rst Outdated Show resolved Hide resolved
spec/aliases.rst Outdated Show resolved Hide resolved
spec/aliases.rst Outdated Show resolved Hide resolved
spec/aliases.rst Outdated Show resolved Hide resolved
spec/annotations.rst Outdated Show resolved Hide resolved
spec/generics.rst Outdated Show resolved Hide resolved

foo(x_y, x_y) # Should return (x: int, y: str) -> bool

foo(x_y, y_x) # Could return (__a: int, __b: str) -> bool
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
foo(x_y, y_x) # Could return (__a: int, __b: str) -> bool
foo(x_y, y_x) # Could return (__a: int, __b: str, /) -> bool

Just to really drive home the fact that these are positional only arguments

Copy link
Owner Author

Choose a reason for hiding this comment

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

We shouldn't use the double underscore convention at all though.

Copy link
Contributor

@Daverball Daverball Nov 27, 2023

Choose a reason for hiding this comment

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

Usually I'd agree and my first instinct was to change it to (a: int, b: str, /), however we aren't really defining a function here, we just want to communicate that the signature of the combined ParamSpec should be positional only, so using double underscores in this case I think helps communicate that the name doesn't really matter a bit better. I was also considering (int, str) -> bool, because I think that's how mypy would display that signature, but that only works because mypy uses the qualified names for the types to disambiguate them from parameter names.

Ideally we would have a common way to describe signatures that we can also use in Callable or other generics with ParamSpec where the positional only arguments don't have a name at all, the mixed ones might and keyword only arguments do.

Copy link
Owner Author

Choose a reason for hiding this comment

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

I feel we shouldn't use the double underscore convention for pos-only parameters in the spec at all: it's a historical hack that has become obsolete now that Python 3.7 is dead. Typeshed will soon stop using it. Users shouldn't need to see it anywhere.

You make good points though; I need to look at this example more closely to decide what I'd want here.

Copy link
Contributor

@Daverball Daverball Nov 27, 2023

Choose a reason for hiding this comment

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

Yeah, another thing I considered was (_: int, _: str, /) -> bool, although that would be invalid syntax once you would try it in a def.

Copy link
Owner Author

Choose a reason for hiding this comment

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

I'm going to change this in a separate PR.

Copy link
Owner Author

Choose a reason for hiding this comment

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

#17

JelleZijlstra and others added 2 commits November 26, 2023 07:27
Co-authored-by: David Salvisberg <dave@daverball.com>
Co-authored-by: David Salvisberg <dave@daverball.com>
Co-authored-by: David Salvisberg <dave@daverball.com>
spec/generics.rst Outdated Show resolved Hide resolved
Co-authored-by: David Salvisberg <dave@daverball.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants