Skip to content

Conversation

@chipmyersjr
Copy link
Contributor

@chipmyersjr chipmyersjr commented Jul 25, 2020

Add typing for jira provider
issue #9708

^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code change, Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in UPDATING.md.


super().__init__(jira_conn_id=jira_conn_id,
result_processor=field_checker_func,
*args,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Remove *args to avoid mypy issue:
python/mypy#6799
#9784

Copy link
Member

@mik-laj mik-laj Jul 26, 2020

Choose a reason for hiding this comment

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

We should remove args in more operators:
#9942

Copy link
Member

@mik-laj mik-laj Jul 26, 2020

Choose a reason for hiding this comment

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

Can you help me with that too in a saperate PR? That way, other people won't face the problem you had.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure will look into soon.

result_processor=None,
get_jira_resource_method=None,
jira_conn_id: str = 'jira_default',
jira_method: Any = None,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
jira_method: Any = None,
jira_method: Optional[str] = None,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Made this change but requires type hint on line 83 because getattr doesn't like getting a None passed as argument

Copy link
Member

Choose a reason for hiding this comment

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

This parameter seems to be required. If the user does not pass this argument, they will always have an error.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks...changed parameter to required

Comment on lines 49 to 50
jira_conn_id: str = 'jira_default',
jira_method: Optional[str] = None,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
jira_conn_id: str = 'jira_default',
jira_method: Optional[str] = None,
jira_method str,
jira_conn_id: str = 'jira_default',

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks made this change

@chipmyersjr
Copy link
Contributor Author

hi @mik-laj, should i update the sensor tests to pass method name parameter so your changes to the sensor will pass the tests?

@mik-laj
Copy link
Member

mik-laj commented Jul 29, 2020

Yes. Please do it. :-D

@chipmyersjr chipmyersjr requested a review from mik-laj July 29, 2020 04:13
@mik-laj mik-laj merged commit ba2d640 into apache:master Jul 29, 2020
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