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

Adding Typehints for jira.py #1418

Draft
wants to merge 15 commits into
base: master
Choose a base branch
from

Conversation

bandophahita
Copy link
Contributor

Since adding typehints to the entire repo would make for a VERY large PR, I'm breaking up the changes mostly by module. This PR focuses on adding hints to jira.py

@bandophahita bandophahita changed the title Adding Typehints Adding Typehints for jira.py Jun 12, 2024
@@ -61,7 +61,7 @@ def each(self, q=None, sort=None):

return

def get(self, uuid):
def get(self, uuid: str): # type: ignore[override]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The signature for get in AtlassianRestAPI doesn't match here. So we need to override.

requirements.txt Outdated
requests>=2.8.1
six
oauthlib
requests_oauthlib
requests-oauthlib
Copy link
Contributor Author

Choose a reason for hiding this comment

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

not sure why this wasn't included as it's used in the library.

types-Deprecated
types-requests
types-six
types-beautifulsoup4
Copy link
Contributor Author

Choose a reason for hiding this comment

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

All of the above get used in tox so it only made sense to include them in the dev reqs.

trailing: bool | None = None,
absolute: bool = False,
advanced_mode: bool = False,
) -> T_resp:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

For those unfamiliar with how overloads work this can look confusing. Essentially this allows mypy to understand that certain arguments change the output type of the method. Due to some of the of the calls using positional arguments forces us to include additional overrides. I wish there was an easier way to make this work with mypy.

absolute=False,
advanced_mode=False,
):
path: str,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not adding the override variations here since patch doesn't get used in jira.py.

pyproject.toml Outdated
@@ -7,7 +7,6 @@
[tool.black]
target-version = ['py36', 'py37', 'py38', 'py39']
line-length = 120
include_trailing_comma = false
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This isn't a valid configuration parameter.

data=data,
flags=flags,
absolute=absolute,
response = cast(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Overall there is a problem with the current design of get in that it could return a dictionary or None (same with the other base methods post, put, and delete). Mypy sees this and complains. casting the return of these calls will avoid mypy errors, but this is really just masking the issue. In theory the code should be doing a check to see if something returned.

Copy link

codecov bot commented Jun 12, 2024

Codecov Report

Attention: Patch coverage is 79.06977% with 18 lines in your changes missing coverage. Please review.

Project coverage is 32.06%. Comparing base (ffb3fc0) to head (50dd19f).

Files Patch % Lines
atlassian/rest_client.py 76.92% 18 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1418      +/-   ##
==========================================
- Coverage   34.38%   32.06%   -2.32%     
==========================================
  Files          46       47       +1     
  Lines        8586     8337     -249     
  Branches     1601     1197     -404     
==========================================
- Hits         2952     2673     -279     
- Misses       5511     5548      +37     
+ Partials      123      116       -7     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

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.

1 participant