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 errors #108

Merged
merged 22 commits into from
Nov 22, 2022
Merged

mypy errors #108

merged 22 commits into from
Nov 22, 2022

Conversation

nreinicke
Copy link
Collaborator

@nreinicke nreinicke commented Oct 28, 2022

Closes #90 by picking off the large list of mypy errors.

@nreinicke nreinicke marked this pull request as draft October 28, 2022 23:35
@nreinicke nreinicke marked this pull request as ready for review October 28, 2022 23:38
@robfitzgerald
Copy link
Collaborator

nice. i know you didn't call this ready but i loaded it up and found all tests pass and mypy says "Found 114 errors in 32 files (checked 204 source files)". that's good progress.

when i run $ mypy tests it tells me There are no .py[i] files in directory 'tests' which is wonksville usa, then i try $ mypy . and i get Found 114 errors in 32 files (checked 207 source files) so it's picking up a few more, but not all of the files in tests/. have you figured out a work-around?

@nreinicke
Copy link
Collaborator Author

Thanks for looking through this! I actually explicitly excluded the tests from mypy so I could focus on the core package but we could remove that and start working on all the errors.

@nreinicke
Copy link
Collaborator Author

Also, if you do mypy --install-types I think you'll see more errors as mypy installs the types for various packages that aren't installed by default.

@nreinicke
Copy link
Collaborator Author

Phew okay, I've gotten through all the mypy errors and this is now ready for review.

I've also added a new CI step that runs mypy to check for type errors.

@nreinicke nreinicke requested review from a user and jhoshiko November 12, 2022 00:02
@robfitzgerald
Copy link
Collaborator

robfitzgerald commented Nov 22, 2022

@nreinicke i'm not sure why, but, for some reason, when i sync your branch, i don't have the fix for vehicle_state_ops that _ignore_s the type check for vehicle_state.route. my copy of your branch:

    elif not hasattr(vehicle.vehicle_state, "update_route"):
        return (
            SimulationStateError(
                f"vehicle state does not have update_route method; context {context}"
            ),
            None,
        )
    else:
        route = vehicle.vehicle_state.route  # throws mypy error

that's even after doing a fresh clone of the repo and pulling to the ndr/90-mypy-errors branch. 🤔

edit: oh, wait, just stepped through the file changes, and it doesn't look like there's a change on line 190. here's the error message i get:

(hive) rfitzger-36698s:hive rfitzger$ mypy . --ignore-missing-imports
nrel/hive/state/vehicle_state/vehicle_state_ops.py:190: error: "VehicleState" has no attribute "route"
Found 1 error in 1 file (checked 243 source files)

thank you for this heroic effort!

@robfitzgerald robfitzgerald self-requested a review November 22, 2022 16:09
Copy link
Collaborator

@robfitzgerald robfitzgerald left a comment

Choose a reason for hiding this comment

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

i had an issue but after upgrading my version of mypy it was resolved.

this is super awesome when mixed with the CI call. thank you!!! 🐝

@nreinicke nreinicke merged commit 5814302 into main Nov 22, 2022
@nreinicke nreinicke deleted the ndr/90-mypy-errors branch November 22, 2022 16:40
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.

Fix mypy errors
2 participants