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.

Sign up for GitHub

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

Jump to bottom

Add typing for tmt/steps/finish/ansible.py #1300

Merged
merged 1 commit into from
Aug 15, 2022
Merged

Add typing for tmt/steps/finish/ansible.py #1300

merged 1 commit into from
Aug 15, 2022

Conversation

guy9050
Copy link
Contributor

@guy9050 guy9050 commented Jun 26, 2022

No description provided.

@guy9050
Copy link
Contributor Author

guy9050 commented Jun 26, 2022

@thrix @happz I am not sure how much this file needs typing, but it was listed in the issue #1182

@guy9050 guy9050 mentioned this pull request Jun 26, 2022
Closed
44 tasks
@thrix thrix added the code | type annotations Related to type annotations and type cleanup label Jun 27, 2022
@thrix
Copy link
Collaborator

thrix commented Jun 27, 2022

@guy9050 please update also mypy.ini

@guy9050 guy9050 requested review from happz and thrix July 3, 2022 08:41
@guy9050
Copy link
Contributor Author

guy9050 commented Jul 10, 2022

@happz @psss @thrix
please review

thrix
thrix approved these changes Jul 19, 2022
View reviewed changes
Copy link
Collaborator

@thrix thrix left a comment

Choose a reason for hiding this comment

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

LGTM

@guy9050 guy9050 force-pushed the typing_finish_ansible.py branch 2 times, most recently from f8f4b2d to 06b9864 Compare July 21, 2022 12:27
psss
psss approved these changes Jul 22, 2022
View reviewed changes
Copy link
Collaborator

@psss psss left a comment

Choose a reason for hiding this comment

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

Looks good but is blocked on #1302, right?

@psss psss added the status | blocked The merging of PR is blocked on some other issue label Jul 22, 2022
@thrix
Copy link
Collaborator

thrix commented Jul 27, 2022

Looks good but is blocked on #1302, right?

yeah, I would wait for it

@thrix thrix added this to the 1.17 milestone Aug 8, 2022
@thrix thrix self-assigned this Aug 8, 2022
@thrix thrix mentioned this pull request Aug 10, 2022
Merged
@psss
Copy link
Collaborator

psss commented Aug 15, 2022

Rebased on the latest main and got the following errors:

tmt/steps/finish/ansible.py:6: error: Unused "type: ignore" comment
tmt/steps/finish/ansible.py:35: error: Incompatible types in assignment (expression has type "Callable[[Optional[Type[Command]]], Command]", base class "FinishPlugin" defined the type as "Callable[[str, Optional[Type[Command]]], Command]")

So the first type: ignore seems not needed anymore, but the explicit class method assignment makes mypy unhappy. @happz, any ideas what's wrong there?

@happz
Copy link
Collaborator

happz commented Aug 15, 2022 

tmt/steps/finish/ansible.py:35: error: Incompatible types in assignment (expression has type "Callable[[Optional[Type[Command]]], Command]", base class "FinishPlugin" defined the type as "Callable[[str, Optional[Type[Command]]], Command]")

Seems to be this issue, python/mypy#6700, or at least something closely related. I'd add a comment with the link to the issue + # type: ignore to suppress the warning.

@psss psss merged commit eb53613 into main Aug 15, 2022
@psss psss deleted the typing_finish_ansible.py branch August 15, 2022 14:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@psss psss psss approved these changes

@thrix thrix thrix approved these changes

@FrNecas FrNecas Awaiting requested review from FrNecas

@lukaszachy lukaszachy Awaiting requested review from lukaszachy lukaszachy is a code owner

@happz happz Awaiting requested review from happz happz is a code owner

Assignees

@thrix thrix

Labels
code | type annotations Related to type annotations and type cleanup status | blocked The merging of PR is blocked on some other issue
Projects
None yet
Milestone
1.17
Development

Successfully merging this pull request may close these issues.
4 participants
@guy9050 @thrix @psss @happz