GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-104050: Argument clinic: Add annotations to the `LandMine` class by AlexWaygood · Pull Request #104648 · python/cpython

Conversation

@AlexWaygood

@AlexWaygood AlexWaygood commented May 19, 2023 •
edited by bedevere-bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

When type checkers see that a class has a __getattr__ or __getattribute__ method, they'll assume that there are things going on with that class that are too magical for them to understand. As such, they will refrain from emitting any errors relating to setting or retrieving attributes from instances of that class. That's basically the opposite of what we want here (there'll be a runtime exception from any attribute access on an instance of LandMine). As such, it's much better to pretend to the type checkers that the __getattribute__ method doesn't exist.

  • Issue: Add type annotations to clinic.py #104050

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
erlend-aasland reacted with eyes emoji
All reactions
  • 👀 1 reaction
@AlexWaygood
pythongh-104050: Argument clinic: Add annotations to the LandMine c…
5b0a76b 
…lass
@AlexWaygood AlexWaygood added the skip news label May 19, 2023
@AlexWaygood
AlexWaygood requested a review from erlend-aasland May 19, 2023 11:59
@bedevere-bot bedevere-bot added the awaiting core review label May 19, 2023
@bedevere-bot bedevere-bot mentioned this pull request May 19, 2023
Add type annotations to clinic.py #104050
Closed
@AlexWaygood
comment
75abb87
@erlend-aasland
erlend-aasland requested review from sobolevn and removed request for erlend-aasland May 20, 2023 21:07
@erlend-aasland erlend-aasland mentioned this pull request May 20, 2023
gh-104050: Add more type annotations to Argument Clinic #104631
Merged
sobolevn
sobolevn reviewed May 20, 2023
View reviewed changes

@sobolevn sobolevn left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Yes, good hack!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@AlexWaygood

AlexWaygood commented May 21, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

This is the best thing to do if we want to have strict typing in clinic.py, but I wonder if we do? The original motivation for adding type hints was to improve maintainability and readability, and this change possibly works against that. The original justification for running a type checker in CI was to have a means of verifying the accuracy of the annotations we were adding, not necessarily because we wanted the code to be strictly type-checked.

Thoughts @erlend-aasland? I don't want to be introducing "typing hacks" into clinic.py if you disagree with the principle :)
erlend-aasland reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@erlend-aasland

erlend-aasland commented May 21, 2023

Copy link
Copy Markdown
Contributor

Strict type-checking is not a goal at the moment. I agree that this does not contribute to increased readability, so I say we drop it.
AlexWaygood reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@AlexWaygood

AlexWaygood commented May 21, 2023

Copy link
Copy Markdown
Member Author

Strict type-checking is not a goal at the moment. I agree that this does not contribute to increased readability, so I say we drop it.

Sounds good. Thanks!
erlend-aasland reacted with heart emoji
All reactions
  • ❤️ 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@AlexWaygood AlexWaygood closed this May 21, 2023
@AlexWaygood
AlexWaygood deleted the argclinic-getattrs branch May 21, 2023 08:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sobolevn sobolevn sobolevn left review comments

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

awaiting core review skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

@AlexWaygood @erlend-aasland @sobolevn @bedevere-bot
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.