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

Resolve mypy errors #28

Merged
merged 5 commits into from Oct 1, 2021
Merged

Conversation

underchemist
Copy link
Contributor

@underchemist underchemist commented Sep 23, 2021

Description

Please include a summary of the change. Please also include relevant motivation and context. List any dependencies that are required for this change.

Depends on #26
Fixes #5, #27

Address errors produced by mypy configuration in #26. Includes

  • Adds no_warn_no_return and no_strict_optional configuration options
  • Updates to typing annotations where appropriate (does not affect runtime)
  • Small changes to runtime code to help mypy infer correct return type for union of variable length tuple(see Unpacking tuples of variable length python/mypy#1178 (comment))
  • As a last resort ,suppression of errors with # type: ignore

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

Instructions

Explain what someone needs to do in order to test the functionality of the changes.

pre-commit install
pre-commit run --all-files

mypy should pass

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

  • pre-commit ran locally without error

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

@underchemist
Copy link
Contributor Author

👋 @captaincoordinates @cuttlefish, @JBurkinshaw suggested you might be good candidates to give this a review, if you have time it's not high priority.

You can see the original errors from main branch after disabling ignore_all_errors in pyproject.toml and running mypy -p oaff.

@underchemist underchemist marked this pull request as ready for review September 27, 2021 20:27
@captaincoordinates
Copy link
Collaborator

@underchemist I'm happy to take a look but just about to leave town for a few days. If it can wait I can look later in the week

@underchemist
Copy link
Contributor Author

@captaincoordinates sounds good!

Copy link
Collaborator

@JBurkinshaw JBurkinshaw left a comment

Choose a reason for hiding this comment

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

I'm seeing an error when I run pre-commit run --all-files:

oaff/fastapi/gunicorn/gunicorn.conf.py:5: error: Module has no attribute "sched_getaffinity"
Found 1 error in 1 file (checked 107 source files)

Other than that, it makes sense to me in terms of making mypy happy. I think @captaincoordinates will be in a better position to comment on some of the modifications and whether there are preferred alternatives.

Copy link
Collaborator

@captaincoordinates captaincoordinates left a comment

Choose a reason for hiding this comment

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

This looks great and the effort is much appreciated. I just have one change request and one suggestion, which you're welcome to disregard.

@captaincoordinates
Copy link
Collaborator

@underchemist from your branch I am unable to successfully run scripts/cibuild. Could you please confirm if this is a local problem for me, or whether you also see issues?

All errors seem to be related to pytz

exclude non-source code directories for mypy

add specific error code to # type: ignore
@underchemist
Copy link
Contributor Author

@captaincoordinates your error should be resolved by calling mypy --install-types. I've also just rebased onto main, which has #29 which adds the --install-types as a configuration option, can you try again?

@captaincoordinates
Copy link
Collaborator

@JBurkinshaw I'll leave the merge to you.
@underchemist the rebase from main addressed the mypy issues I was seeing. If it was necessary for me to execute mypy --install-types then that command should be included in scripts/setup, but I did not actually end up having to do this.

@underchemist
Copy link
Contributor Author

thanks @captaincoordinates, indeed --install-types will be used automatically now due to the configuration set in pyproject.toml.

@JBurkinshaw
Copy link
Collaborator

Looks good. Making the class names more explicit is a good move.

@JBurkinshaw JBurkinshaw closed this Oct 1, 2021
@JBurkinshaw JBurkinshaw reopened this Oct 1, 2021
@JBurkinshaw JBurkinshaw merged commit 766a7a5 into microsoft:main Oct 1, 2021
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.

Implement mypy type validation
3 participants