Skip to content

Idiomatic type check#1991

Merged
mhammond merged 10 commits intomhammond:mainfrom
Avasam:idiomatic-type-check
Jul 10, 2023
Merged

Idiomatic type check#1991
mhammond merged 10 commits intomhammond:mainfrom
Avasam:idiomatic-type-check

Conversation

@Avasam
Copy link
Copy Markdown
Collaborator

@Avasam Avasam commented Dec 7, 2022

This is the third PR in a series concerning code cleanup. With the final goal to make basic type-checking validation of public methods possible, easing the addition of 3.7+ type annotations.

  • Compare None using is rather than ==
  • Do static type comparisons using isinstance, which is the idiomatic way of type-checking in Python. Type-checkers also support type inference this way.

@Avasam Avasam force-pushed the idiomatic-type-check branch 3 times, most recently from edc7b89 to ea9c389 Compare December 7, 2022 21:02
@Avasam Avasam force-pushed the idiomatic-type-check branch from ea9c389 to eacae96 Compare December 7, 2022 21:12
Copy link
Copy Markdown
Owner

@mhammond mhammond left a comment

Choose a reason for hiding this comment

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

It's going to take some time to go through this manually - and the import a plus is None changes make it more difficult. We also need to be careful because isinstance and checking type() == ... have different semantics in the case of subclasses, which will not usually be a problem, but might sometimes.

dllid may be None, a dll object, or a string with a dll name"""
# must take a reference to the DLL until InitDialog.
self.dll = dllFromDll(dllid)
if type(id) == type([]): # a template
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

isn't this (and the similar change below) wrong?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I fixed up the PR and removed the changes that shouldn't have been in there. Ready to review :)

@Avasam
Copy link
Copy Markdown
Collaborator Author

Avasam commented Feb 21, 2023

The import changes should be split off in #1986 I didn't realize this PR still contained them.
I'd definitely prefer undoing the import changes from this PR before you have to review it.

@Avasam Avasam requested a review from mhammond February 27, 2023 02:33
@Avasam
Copy link
Copy Markdown
Collaborator Author

Avasam commented Jul 8, 2023

@mhammond Even if they both fall under "idiomatic type checks", would you prefer if I split off None checks?

Copy link
Copy Markdown
Owner

@mhammond mhammond left a comment

Choose a reason for hiding this comment

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

This looks great, thanks.

@mhammond mhammond merged commit 83e7f29 into mhammond:main Jul 10, 2023
@Avasam Avasam deleted the idiomatic-type-check branch July 10, 2023 15:48
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.

2 participants