add typehints and allow google docstrings #1023

adehad · 2021-05-03T13:30:16Z

Summary of Changes

Add typehints, coaxing mypy where required - i.e. typing.cast ,isinstance() and as a last resort assert
Re-order JIRAError argument order to reflect how the base Exception class is used, i.e. first argument is a string, prevent misuse of status_code for a string of an error message
Allow "Google style" docstrings, allowing for more legible docstrings
The basic_use.py no longer worked, since GDPR the emailAddress property has been removed, changing the field used in the example to .key at least makes it run fine.
Partially (maybe fully?) address Disabling logging is not working #1009 in order to satisfy mypy, refer to commit: "bugfix: respect logging argument" (cc504a1).
Removed logging parameter and switched back to standard module logging #329 review comments were checked, I don't feel the logging changes made here are particularly contentious - they didn't intend to be. I think a further PR should be raised if anything more needs to be done - as the changes here we mainly for the mypy fixes.
The implementation chosen imports logging as _logging, this should further prevent a developer accidentally using the raw logging module. Exposing the logger under self.log may also allow user to use the logger if they monkey patch or base class of the JIRA class.

~~This branch is currently based of the isort branch~~ now rebased

Todo:

~~Document logging bug fixed on the client.py side, may need changes to the other classes, e.g. resilient session~~ should be a separate PR if anything further is required than what was added to fix mypy
Document fix of JIRAError class, argument order causing misuse
~~Convert str = None to Optional[str] = None if the function has an actual none check~~ not required as inferred by mypy, could maybe check they are added to the docstrings, but might take that up in another PR, a bit burnt out as I severely under estimated the work required here haha
Check the actual docs build looks ok
do some more google style docstrings if you aren't tired of them yet, resources.py has a few left
where possible, convert assertions to properly typehinted variables
cleanup the commit history, no point separating by files as they all need to work together for mypy to be happy

tuukkamustonen

Took a glance at this, then realized it's a massive diff (as would be expected). As I'm not familiar with the internals of the library, I stopped :)

I see there are 0 checks for PRs, which is a bit scary, but if it passes mypy (and other linters) checks, it's a good indication.

The most vital thing (imo) is to have the public API annotations in place (and detailed enough). Internals aid in development, but are normally not needed for users of the library. But this PR covering everything is a nice thing, for sure.

What should be added, is py.typed file, so that mypy can actually read the types from the package (I don't know why they require that, IDEs don't need it). Can you add that? See #1025.

tuukkamustonen · 2021-05-11T13:21:59Z

docs/conf.py

+
+napoleon_google_docstring = True
+napoleon_numpy_docstring = False # Explicitly prefer Google style docstring
+napoleon_use_param = True # for type hint support


napoleon_google_docstring and napoleon_use_param already default to True in plugin. Can be removed?

see there are 0 checks for PRs, which is a bit scary, but if it passes mypy (and other linters) checks, it's a good indication.

Regarding the checks, this branch will definitely wait for the CI to pass before merging.

The background is currently the CI test suite is only run on PR's directed at the main branch, as currently this branch targetting an unmerged branch (the isort branch, which this branch is based off of) they aren't being run.

I have run the tests locally so have some confidence it will pass.

What should be added, is py.typed file, so that mypy can actually read the types from the package

I did not know this, I will take a look. Thanks for raising.

napoleon_google_docstring and napoleon_use_param already default to True in plugin. Can be removed?

This is true, it is just a personal preference to have these written explicitly so they are not accidentally changed in the future. Happy to remove if there is a preference either way.

Took a glance at this, then realized it's a massive diff (as would be expected). As I'm not familiar with the internals of the library, I stopped :)

Luckily 90% of the diff (probably ) is just docstrings hahaha. But yeah, I do regret creating such a monster of a PR, as the files and functions depend on one another it was hard to pick a stopping point, as mypy would be throwing warnings left and right.

it was hard to pick a stopping point, as mypy would be throwing warnings left and right.

Completely understandable.

It's good to get the type hints! Even if the hints were invalid, the actual code should not break. So, if a few issues slip in on first go (despite passing 100% of checks), it's not the end of the world 💣.

py.typed file added, branch also rebased on to master confirming tests pass.

tuukkamustonen · 2021-05-13T07:45:18Z

jira/resilientsession.py


- def options(self, url, **kwargs):
- return self.__verb("OPTIONS", url, **kwargs)
+ def options(self, url: Union[str, bytes], **kwargs) -> Response:


I think you could use AnyStr for Union[str, bytes]. Just slightly shorter.

Unfortunately we can't use AnyStr in this case. This is because this class is a subclass of Session, so the typehints need to be consistent with the parent class. Technically we could add a # type: ignore[override], but I felt it unnecessary.

Example of mypy failure

Suggested change

def options(self, url: Union[str, bytes], **kwargs) -> Response:

def options(self, url: AnyStr, **kwargs) -> Response:

jira\resilientsession.py:221: error: Signature of "options" incompatible with supertype "Session" Found 1 error in 1 file (checked 19 source files)

studioj · 2021-05-13T16:31:20Z

i see you're changing the way the documentation is generated

maybe check this PR #1039

studioj · 2021-05-13T16:52:24Z

jira/client.py

- :param ignore_epics: ignore any issues listed in ``issue_keys`` that are epics. (Default: True)
- :type ignore_epics: bool
+ Args:
+ epic_id (int): The ID for the epic where issues should be added.


#968 mentions a mistake here maybe take it along

close the issue if you take it along

I've added it as a linked issue, I'll close after this PR is merged

studioj · 2021-05-14T01:00:33Z

examples/basic_auth.py


 # Find all issues reported by the admin
-issues = jira.search_issues("assignee=admin")
+issues = cast(ResultList[Issue], jira.search_issues("assignee=admin"))


maybe explain this a little bit more => why cast?

studioj · 2021-05-14T01:01:49Z

examples/basic_use.py


 # Or modify the List of existing labels. The new label is unicode with no
 # spaces
+assert issue.fields.labels # help mypy


just for my interest what do you mean with help mypy?

So mypy thinks that the issue.fields.labels can be None, but it has certain ways to infer the type of a variable. This can be via cast(), isinstance() or assert statements - refer: https://mypy.readthedocs.io/en/stable/casts.html

studioj · 2021-05-14T01:02:57Z

examples/basic_use.py

 issue = jira.issue("JRA-1330")
 issue2 = jira.issue("XX-23") # could also be another instance
-jira.add_remote_link(issue, issue2)
+jira.add_remote_link(str(issue.id), issue2)


why cast to str? issue.id is a string already..

Yep you are completely right. The original typehint was an Optional[int] which was incorrect, it is always present, and is a str. I verified this both from the API documentation and by checking examples in jira.atlassian.com

It also helped me remove the assertion in this file

adehad · 2021-05-14T17:58:08Z

jira/resources.py

- self.key = None
- """ :type: str """
+ self.fields: Optional[Issue._IssueFields] = None
+ self.id: Optional[int] = None


Referring to both:
https://docs.atlassian.com/software/jira/docs/api/REST/8.13.6/#api/2/issue-getIssue

and

https://developer.atlassian.com/cloud/jira/platform/rest/v2/api-group-issues/#api-rest-api-2-issue-issueidorkey-get

This Issue id field is a str

ssbarnea · 2021-05-15T12:14:23Z

Replase resolve conflicts.

adehad · 2021-05-15T12:30:39Z

@ssbarnea fixed conflicts, happy to address the Codacy failure in a separate PR, seems like we should add pylint and mccabe to our lint step ?

Looks like a few of them are already picked up by mypy and I've added the ignore[override] to keep mypy happy.

ssbarnea · 2021-05-15T12:45:20Z

@ssbarnea fixed conflicts, happy to address the Codacy failure in a separate PR, seems like we should add pylint and mccabe to our lint step ?

Looks like a few of them are already picked up by mypy and I've added the ignore[override] to keep mypy happy.

We already have pylint as part of pre-commit but maybe we have too many includes. Codacy can be ignored for the moment as what matters regards to linting is to pass pre-commit. Obviously that any change that goes towards making codacy happier is appreciated.

* chore: allow google docstring * bugfix: respect logging argument * bugfix: correct usage of JIRAError, add typehints * feature: add typehints and coax mypy * example, add _Comment helper * feature: add py.typed for PEP-561 compliance * bugfix: fix epic_id typehint, closes pycontribs#968

adehad added bug enhancement docs labels May 3, 2021

adehad changed the base branch from master to feature/isort May 3, 2021 13:30

adehad force-pushed the feature/typehints_and_google_docstrings branch 2 times, most recently from 9bbdd81 to 3e7e3b7 Compare May 9, 2021 12:32

adehad marked this pull request as ready for review May 9, 2021 12:34

adehad force-pushed the feature/typehints_and_google_docstrings branch 2 times, most recently from a2fc82b to 824aa1a Compare May 9, 2021 17:47

studioj mentioned this pull request May 11, 2021

Type hints & PEP 561 compatible package #1025

Closed

tuukkamustonen reviewed May 11, 2021

View reviewed changes

delete-merged-branch bot deleted the branch master May 12, 2021 12:46

adehad added 3 commits May 12, 2021 18:04

chore: allow google docstring

33344dd

bugfix: respect logging argument

cc504a1

bugfix: correct usage of JIRAError, add typehints

096e048

adehad force-pushed the feature/typehints_and_google_docstrings branch from 824aa1a to 7937b84 Compare May 12, 2021 17:05

adehad changed the base branch from feature/isort to master May 12, 2021 17:05

adehad force-pushed the feature/typehints_and_google_docstrings branch from 7937b84 to ba44342 Compare May 12, 2021 17:19

feature: add typehints and coax mypy

c30ac81

adehad force-pushed the feature/typehints_and_google_docstrings branch from ba44342 to 0d58e50 Compare May 12, 2021 17:44

tuukkamustonen reviewed May 13, 2021

View reviewed changes

studioj reviewed May 13, 2021

View reviewed changes

adehad linked an issue May 13, 2021 that may be closed by this pull request

Client API, epic_id in add_issues_to_epic() should be string #968

Closed

studioj reviewed May 14, 2021

View reviewed changes

studioj mentioned this pull request May 14, 2021

Shadowing outer "logging" in client.py #755

Closed

This was linked to issues May 14, 2021

Type hints & PEP 561 compatible package #1025

Closed

Disabling logging is not working #1009

Closed

adehad linked an issue May 14, 2021 that may be closed by this pull request

Shadowing outer "logging" in client.py #755

Closed

adehad commented May 14, 2021

View reviewed changes

adehad added 3 commits May 14, 2021 19:24

example, add _Comment helper

b5cad78

feature: add py.typed for PEP-561 compliance

037359d

bugfix: fix epic_id typehint, closes #968

998b869

adehad force-pushed the feature/typehints_and_google_docstrings branch from 1da4a70 to 998b869 Compare May 14, 2021 18:27

adehad linked an issue May 14, 2021 that may be closed by this pull request

Expose documentation #725

Closed

adehad mentioned this pull request May 14, 2021

Expose documentation #725

Closed

Merge branch 'master' into feature/typehints_and_google_docstrings

d974c75

ssbarnea approved these changes May 15, 2021

View reviewed changes

ssbarnea merged commit 6e51395 into master May 15, 2021

delete-merged-branch bot deleted the feature/typehints_and_google_docstrings branch May 15, 2021 12:45

eljulians mentioned this pull request Jun 1, 2021

Add (some) missing type hints for _IssueFields #1063

Merged

andy-maier mentioned this pull request Sep 23, 2021

SSLError raised by requests causes follow-on AttributeError #1158

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

add typehints and allow google docstrings #1023

add typehints and allow google docstrings #1023

Uh oh!

adehad commented May 3, 2021 •

edited

Loading

tuukkamustonen left a comment

tuukkamustonen May 11, 2021

adehad May 11, 2021

tuukkamustonen May 11, 2021

adehad May 12, 2021

tuukkamustonen May 13, 2021 •

edited

Loading

adehad May 14, 2021

studioj commented May 13, 2021

studioj May 13, 2021

studioj May 13, 2021

adehad May 14, 2021

studioj May 14, 2021

studioj May 14, 2021

adehad May 14, 2021

studioj May 14, 2021

adehad May 14, 2021

adehad May 14, 2021

ssbarnea commented May 15, 2021

adehad commented May 15, 2021 •

edited

Loading

ssbarnea commented May 15, 2021

Labels

5 participants

	def options(self, url: Union[str, bytes], **kwargs) -> Response:
	def options(self, url: AnyStr, **kwargs) -> Response:

Uh oh!

add typehints and allow google docstrings #1023

add typehints and allow google docstrings #1023

Uh oh!

Conversation

adehad commented May 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary of Changes

Todo:

tuukkamustonen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tuukkamustonen May 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Example of mypy failure

studioj commented May 13, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ssbarnea commented May 15, 2021

adehad commented May 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

ssbarnea commented May 15, 2021

Labels

5 participants

adehad commented May 3, 2021 •

edited

Loading

tuukkamustonen May 13, 2021 •

edited

Loading

adehad commented May 15, 2021 •

edited

Loading