Add (some) missing type hints for _IssueFields

[eljulians]

Description

Tested with mypy 0.782.

Pull request #1023 added some type hints, which is great, but didn't define all of them, which now makes mypy unhappy about it.

Note: this PR probably doesn't add all of them either, but it does add some of them that do exist (most notably, fields like summary or created). I haven't found in the Jira REST API documentation a specific list of the fields that have a required value, so the types added by this PR are based in the errors of our codebase, intuition and requests examples provided by Jira docs. I'd be happy to add more fields if necessary.

Rationale

Because mypy assumes the type Any for when there's no type hint, the solution should be to either to define every class attribute with its type, or define no types at all. This design by mypy is intentional for backwards compatibility (among other reasons). Having partially defined types can only break existing set ups.

Minimal reproducible example

from jira import JIRA


jira_client = JIRA('http://jira.test')
issue = jira_client.issue('TEST-1')
print(issue.fields.summary)

venv ❯❯❯ mypy script.py
script.py:7: error: "_IssueFields" has no attribute "summary"
Found 1 error in 1 file (checked 1 source file)

[adehad]

Great PR.

I imagine this will also be a pain for basically every customfield property too.

Two options I'm thinking:

1. Use an Any typehint self.fields: Any, removing _IssueFields
2. Inherit Any in the _IssueFields class during type checking

The inherit Any is a bit hacky, so perhaps we will just use Any in the future, but:

if TYPE_CHECKING:
    from jira.client import JIRA

    MyAny = Any
else:

    class MyAny(object):
        pass

Then:

    class _IssueFields(MyAny):

Otherwise the fields you've added seem fine, some of them I can't see on my instance, or the https://jira.atlassian.com instance, but you've set them as optional so don't think it'll hurt to keep them in.

@studioj any thoughts ?

[eljulians]

Thanks @adehad for the feedback.

Regarding the customfields, the types I've defined have been made under the assumption that they'd be common for any Jira instance (of course my assumptions may be wrong, and the Jira documentation doesn't really help much for this, at least for what I could find), and that, if someone was looking for some custom fields, they would have to check that the variable is bound.

But if subclassing Any is a more feasible solution, I'm also good with that too

[adehad]

But if subclassing Any is a more feasible solution, I'm also good with that too

Great, let's wait a little bit in case anyone else has any thoughts on the matter.

If I was being picky I would suggest doing the raw type casting for all the Resource subclasses, but that can also be done as a separate PR

[eljulians]

@adehad

Great, let's wait a little bit in case anyone else has any thoughts on the matter.

That makes sense, let's wait for other opinions

If I was being picky I would suggest doing the raw type casting for all the Resource subclasses, but that can also be done as a separate PR

No issue, I can do it in this PR already! I'll push the changes in a while

[studioj]

Great PR.

I imagine this will also be a pain for basically every customfield property too.

Two options I'm thinking:

1. Use an Any typehint self.fields: Any, removing _IssueFields
2. Inherit Any in the _IssueFields class during type checking

The inherit Any is a bit hacky, so perhaps we will just use Any in the future, but:

if TYPE_CHECKING:
    from jira.client import JIRA

    MyAny = Any
else:

    class MyAny(object):
        pass

Then:

    class _IssueFields(MyAny):

Otherwise the fields you've added seem fine, some of them I can't see on my instance, or the https://jira.atlassian.com instance, but you've set them as optional so don't think it'll hurt to keep them in.

@studioj any thoughts ?

tbh I'm not that acquainted w type hints. :-) I only recently finally made the step to drop legacy python in my professional life.
type hints are an added feature which is on my to learn list :-)

i'm thinking that it looks rather complicated... is this worth the complexity and added maintenance? (not the PR but the above proposal)

[adehad]

@ssbarnea any thoughts on the maintenance of these typehints?

Least maintenance would be self.fields: Any

The MyAny proposal is also technically low maintenance from a typehint point of view, but may break at the mypy level

[eljulians]

@adehad I've added the types for the raw attributes of the rest Resource subclasses, let me know if yuo would address something else!

[adehad]

@julenpardo, great! Although I noticed it looks like you have missed 3 instances at the bottom of the file for: Customer, ServiceDesk, RequestType

[eljulians]

@adehad Thanks for the catch! Already added, also for UnknownResource

[adehad]

Nice, looks good. Now just waiting for a decision on the Any situation from @ssbarnea

Changes now made, no explicit approval as awaiting decision on Any typehint

[eljulians]

Hey folks, any follow up regarding defining types as Any?

[ssbarnea]

I do not have time to look into full details of this but use of Any is almost always a type problem.
It work mentioning that JSON data has a limited number of return data types, so we could narrow it down and avoid use of Any.

[adehad]

Hi @julenpardo let's implement the MyAny suggestion, I've been given merge permissions now, so I should be able to merge this after that.

[eljulians]

I'm not strong opinionated about this. As of me, having no types at all (specially when just some of them were added) would still be fine. I can understand that the argument in favor of Any is a lower maintenance effort, but also its drawbacks.

I originally opened this PR with the intention solely of unbreaking the type checker, so any decision taken here would be okay from my side.

[eljulians]

Thanks @adehad, I posted the previous comment just before seeing yours :) I'll make the changes and ping you back

[eljulians]

@adehad I've made _IssueFields subclass Any for the type checking, but kept the type hints I also defined in the constructor, should we remove them or just leave it as it is?

[adehad]

@julenpardo let me get back to you on the existing typehints, I think we may be able to just leave them in. For now could you merge in the latest master to make sure the pipeline is remains all good?

@ssbarnea regarding the Any type hint - agree, we are basing the json fields on a Dict[str,Any] - which is what we need to rely on until we get recursive type support in mypy: python/typing#182 (comment) and python/mypy#731. We have to fallback to Any here because we support accessing the json dict fields with __getattr__ , so it gets a bit funky, for example if there is a custom field we haven't type hinted

[eljulians]

@adehad Thanks for following up with this, already rebased latest master and addressed the comment

[adehad]

Great. I think this is good.

I wanted to check if VSCode would pick up on the types of _IssueFields despite inheriting from Any during type checking, and I can confirm it does (using PyLance as the language server):

[ssbarnea]

@adehad I personally found MyAny horrible from the naming point of view but on the other hand we can change it later. Indeed when it comes to type hints mypy and behavior of vscode (pylance) are the magic combination. If one of them does complain it is a clear sign we did something wrong.