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

MyPy fixes #1532

Merged
merged 4 commits into from
Nov 30, 2023
Merged

MyPy fixes #1532

merged 4 commits into from
Nov 30, 2023

Conversation

Zeitsperre
Copy link
Collaborator

@Zeitsperre Zeitsperre commented Nov 21, 2023

Pull Request Checklist:

  • This PR addresses an already opened issue (for bug fixes / features)
    • This PR fixes #xyz
  • Tests for the changes have been added (for bug fixes / features)
    • (If applicable) Documentation has been added / updated (for bug fixes / features)
  • CHANGES.rst has been updated (with summary of main changes)
    • Link to issue (:issue:number) and pull request (:pull:number) has been added

What kind of change does this PR introduce?

  • Addresses around 100 very basic mypy typing errors

Does this PR introduce a breaking change?

No, but some call signatures are changed (more precise).

Other information:

https://mypy.readthedocs.io/en/stable/index.html

Sorry, something went wrong.

@Zeitsperre Zeitsperre added the standards / conventions Suggestions on ways forward label Nov 21, 2023
@Zeitsperre Zeitsperre requested a review from aulemahal November 21, 2023 17:52
@Zeitsperre Zeitsperre self-assigned this Nov 21, 2023
@github-actions github-actions bot added sdba Issues concerning the sdba submodule. indicators Climate indices and indicators labels Nov 21, 2023
@Zeitsperre
Copy link
Collaborator Author

Zeitsperre commented Nov 21, 2023

This is failing due to changes in Pandas-related time signature codes. See: #1534

Copy link
Collaborator

@aulemahal aulemahal 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!

I think I would have preferred leaving the default values of robustness_categories in the signature because even though the docstring is verbose, it makes it clear what the fault behaviour is. Not going to die on this hill tho.

Comment on lines +333 to +335
categories: list[str] | None = None,
ops: list[tuple[str, str]] | None = None,
thresholds: list[tuple[float, float]] | None = None,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this because MyPy doesn't like complex default values ?

Copy link
Collaborator Author

@Zeitsperre Zeitsperre Nov 23, 2023

Choose a reason for hiding this comment

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

It's not so much that it's complex values, it's because those values are treated as mutable objects. It's very easy for a user developer to accidentally modify the default arguments during calls, leading to weird behaviour when called again.

https://stackoverflow.com/questions/1132941/least-astonishment-and-the-mutable-default-argument

@github-actions github-actions bot added the approved Approved for additional tests label Nov 23, 2023

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@coveralls
Copy link

Coverage Status

coverage: 90.452%. first build
when pulling c73cbff on mypy
into d382912 on master.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@Zeitsperre Zeitsperre added this to the v0.47.0 milestone Nov 30, 2023
@Zeitsperre Zeitsperre merged commit 453850a into master Nov 30, 2023
@Zeitsperre Zeitsperre deleted the mypy branch November 30, 2023 19:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Approved for additional tests indicators Climate indices and indicators sdba Issues concerning the sdba submodule. standards / conventions Suggestions on ways forward
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants