Skip to content

Conversation

@AlexWaygood
Copy link
Member

@AlexWaygood AlexWaygood commented May 18, 2023

  • Convert unspecified and unknown to be members of a Sentinels enum, rather than instances of bespoke classes.
    • An enum feels more idiomatic here, and works better with type checkers.
    • Convert some == and != checks for these values to identity checks, which are more idiomatic with sentinels.
    • Don't do the same for Null, as this needs to be a distinct type due to its usage in clinic.py.
  • Use object as the annotation for default across clinic.py. default can be literally any object, so object is the correct annotation here.
@AlexWaygood
Copy link
Member Author

Not sure if converting the unspecified and unknown sentinels to become enum members is entirely worth it; now that default is typed as object anywhere, the benefits aren't so obvious. I do think it still makes the code a little cleaner, but you tell me whether you think it's worth the churn @erlend-aasland :)

@erlend-aasland
Copy link
Contributor

Not sure if converting the unspecified and unknown sentinels to become enum members is entirely worth it; now that default is typed as object anywhere, the benefits aren't so obvious. I do think it still makes the code a little cleaner, but you tell me whether you think it's worth the churn @erlend-aasland :)

IMO, the more readable we can get clinic.py, the better. We should thread a little careful here, since test_clinic has pretty low coverage. Some of the paths modified are not tested. However, the CI does make clinic (for the check-generated-files job), and that would blow up if semantics changed for the code in question, so we're fine for now.

@AlexWaygood
Copy link
Member Author

IMO, the more readable we can get clinic.py, the better. We should thread a little careful here, since test_clinic has pretty low coverage. Some of the paths modified are not tested. However, the CI does make clinic (for the check-generated-files job), and that would blow up if semantics changed for the code in question, so we're fine for now.

Fab. Thanks for the review!

@AlexWaygood AlexWaygood merged commit 1c55e8d into python:main May 18, 2023
@AlexWaygood AlexWaygood deleted the sentinels-clinic branch May 18, 2023 22:42
carljm added a commit to carljm/cpython that referenced this pull request May 18, 2023
* main: pythongh-74690: Don't set special protocol attributes on non-protocol subclasses of protocols (python#104622) pythongh-104623: Update Windows installer to use SQLite 3.42.0 (python#104625) pythongh-104050: Add more type annotations to Argument Clinic (python#104628) pythongh-104629: Don't skip test_clinic if _testclinic is missing (python#104630) pythongh-104549: Set __module__ on TypeAliasType (python#104550) pythongh-104050: Improve some typing around `default`s and sentinel values (python#104626) pythongh-104146: Remove unused vars from Argument Clinic (python#104627) pythongh-104615: don't make unsafe swaps in apply_static_swaps (python#104620) pythonGH-104484: Add case_sensitive argument to `pathlib.PurePath.match()` (pythonGH-104565) pythonGH-96803: Document and test new unstable internal frame API functions (pythonGH-104211) pythonGH-104580: Don't cache eval breaker in interpreter (pythonGH-104581)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

3 participants