Skip to content

💄 Added parameter type hints#33

Merged
mbuttner merged 9 commits into
scverse:mainfrom
Ma-Fi-94:main
Oct 14, 2022
Merged

💄 Added parameter type hints#33
mbuttner merged 9 commits into
scverse:mainfrom
Ma-Fi-94:main

Conversation

@Ma-Fi-94
Copy link
Copy Markdown
Contributor

No changes to the program logic itself; added type hints for some function parameters which have not been provided before.

Copy link
Copy Markdown
Collaborator

@mbuttner mbuttner 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 to me, thank you for adding the types!

@mbuttner
Copy link
Copy Markdown
Collaborator

@Ma-Fi-94 I have run the github actions for your PR and the linting fails in the types you added. Can you have a look at the report and address the issues, please?

@mbuttner mbuttner self-requested a review October 14, 2022 06:11
Copy link
Copy Markdown
Collaborator

@mbuttner mbuttner left a comment

Choose a reason for hiding this comment

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

Lint fails.

Reverted (for now) annotations in two helper functions which made mypy trip up. Additionally, changed two assignments (ll. 216 and 504) from "=0" to "=0.0", so that mypy understands we are dealing with floats here. Finally, explicitly annotated variable dx (l. 507) as float, so that mypy doesn't complain when we assign floats to it later (e.g. l. 525).
@mbuttner
Copy link
Copy Markdown
Collaborator

OK, so all of the typing related error messages are resolved, that is great! The remaining fails are due to reformatting of some files. I suggest that you install pre-commit and run it on the pytometry package. The style changes are then automatically applied and you can commit "correct" files.

Comment thread pytometry/plotting/_scatter_density.py Outdated
y_scale: str = "linear",
x_lim: List[float] = [-2 * 1e4, 3 * 1e5],
y_lim: List[float] = [-2 * 1e4, 3 * 1e5],
ax: Optional[mpl.Axes] = None,
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think that you can do from matplotlib.axes import Axes and the replace mpl.Axes by Axes

Copy link
Copy Markdown
Collaborator

@mbuttner mbuttner left a comment

Choose a reason for hiding this comment

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

Import error mpl.Axes.

@Ma-Fi-94
Copy link
Copy Markdown
Contributor Author

Did! The same problem seems to be the case for the colormap too, I'll also change this. (Also sorry for the flooding, I'm still rather new to collaboratve software development, apologies!)

@mbuttner mbuttner changed the title Added some parameter type hints 💄 Added parameter type hints Oct 14, 2022
@mbuttner
Copy link
Copy Markdown
Collaborator

Don't worry, it took me a while to understand the whole pre-commit stuff as well. All checks on your PR are now successful, congrats!

@mbuttner mbuttner merged commit d7bb822 into scverse:main Oct 14, 2022
@Ma-Fi-94
Copy link
Copy Markdown
Contributor Author

Thank you for the guidance!

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.

2 participants