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.

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

Add notify to comment #1387

Merged
merged 5 commits into from
Jun 5, 2022
Merged

Add notify to comment #1387

merged 5 commits into from
Jun 5, 2022

Conversation

studioj
Copy link
Collaborator

@studioj studioj commented Jun 1, 2022

my own implementation of #1008

I didn't want to add the functionality to jira.add_comment since I'm not sure if it has the same way of adding the notification as add_user => data["notification"]='false' OR data["notifyUsers"] = "false"

@studioj
Copy link
Collaborator Author

studioj commented Jun 1, 2022

@adehad how do I ignore this since i don't want to change the api i don't think it's smart to listen to mypy here.
I think it's complaining about the order of the args but if people are using the kwargs as positional args we break the api

jira/resources.py:766: error: Signature of "update" incompatible with supertype "Resource"  [override]
jira/resources.py:766: note:      Superclass:
jira/resources.py:766: note:          def update(self, fields: Optional[Dict[str, Any]] = ..., async_: Optional[bool] = ..., jira: Optional[JIRA] = ..., notify: bool = ..., **kwargs: Any) -> Any
jira/resources.py:766: note:      Subclass:
jira/resources.py:766: note:          def update(self, fields: Optional[Dict[str, Any]] = ..., async_: Optional[bool] = ..., jira: Optional[JIRA] = ..., body: str = ..., visibility: Optional[Dict[str, str]] = ..., is_internal: bool = ..., notify: bool = ...) -> Any

i will add a @typing.no_type_check decorator

@pre-commit-ci pre-commit-ci bot temporarily deployed to cloud June 3, 2022 07:45 Inactive
crimsonxiii
crimsonxiii previously approved these changes Jun 3, 2022
self,
fields: Optional[Dict[str, Any]] = None,
async_: Optional[bool] = None,
jira: "JIRA" = None,

Choose a reason for hiding this comment

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

more for myself, but why the quotes around JIRA?
I didn't see such a variant on https://docs.python.org/3/library/typing.html

Copy link
Contributor

Choose a reason for hiding this comment

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

This is because we only import the JIRA class when type checking, and current Python versions actually try to evaluate the type hint.

Some more reading here: https://mypy.readthedocs.io/en/stable/runtime_troubles.html#annotation-issues-at-runtime

Copy link
Contributor

@adehad adehad 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, one main suggestion about the type hinting

self,
fields: Optional[Dict[str, Any]] = None,
async_: Optional[bool] = None,
jira: "JIRA" = None,
Copy link
Contributor

Choose a reason for hiding this comment

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

This is because we only import the JIRA class when type checking, and current Python versions actually try to evaluate the type hint.

Some more reading here: https://mypy.readthedocs.io/en/stable/runtime_troubles.html#annotation-issues-at-runtime

@studioj studioj force-pushed the add_notify_to_comment branch 4 times, most recently from a655163 to 6ecc3c2 Compare June 5, 2022 20:04
@studioj studioj temporarily deployed to cloud June 5, 2022 20:21 Inactive
@studioj studioj force-pushed the add_notify_to_comment branch from 6ecc3c2 to eac32b7 Compare June 5, 2022 21:08
@studioj studioj temporarily deployed to cloud June 5, 2022 21:21 Inactive
@studioj
Copy link
Collaborator Author

studioj commented Jun 5, 2022

@adehad would be nice if this is released "soon" (maybe together with the other approved PR) my team would really like to use this feature :-)
I don't know the effort to release, glad to help if needed

@adehad adehad merged commit 8bc0ec3 into main Jun 5, 2022
@adehad adehad deleted the add_notify_to_comment branch June 5, 2022 23:07
@adehad
Copy link
Contributor

adehad commented Jun 5, 2022

@studioj yep I think a new release is due soon too! I'll also make progress on my PR, would be great if you can review it once I've made the changes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants