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

Add Mypy? #27

Closed
wants to merge 1 commit into from
Closed

Add Mypy? #27

wants to merge 1 commit into from

Conversation

cc-a
Copy link
Collaborator

@cc-a cc-a commented Jul 3, 2024

Provides the infrastructure for mypy usage via pytest-mypy. Uses a loose configuration that effectively makes typehinting optional. Adds hints to an oauth handler as an example.

I'm not 100% on this. Given the lack of typing in the Invenio and wider Flask ecosystems (Flask itself is typed) there are some questions about how much value we would derive.

Some pros:

  • more informative function signatures - the oauth handler example is fairly pertinent as much of the code in the project will be handler functions plugged into the frameworks, the hints give at least some clue what data structures are being passed in although these are not being checked.
  • Flask itself is typed so may be possible to derive more value on view functions (could consider tightening requirements for relevant modules).

Some cons:

  • lots of modules will need to be added to the ignores in pyproject.toml.
  • hints could end up being actively misleading if not checked.

@cc-a
Copy link
Collaborator Author

cc-a commented Jul 24, 2024

@J4bbi @Steven-Eardley could one of you approve so this can be merged?

@cc-a
Copy link
Collaborator Author

cc-a commented Jul 24, 2024

Ugh... just noticed this was opened against main and we've swapped to using develop. I'll need to close and open an new one.

@cc-a cc-a closed this Jul 24, 2024
@cc-a cc-a mentioned this pull request Jul 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants