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

install mypy + update dev deps + update pre commit + begin type hinting #292

Merged
merged 11 commits into from
Aug 1, 2023

Conversation

dekim24
Copy link
Contributor

@dekim24 dekim24 commented Jul 21, 2023

This is basically the minimum required code in order to get mypy . to pass. In future PR(s) I will work through untyped definitions and add type hints to everything, with the end goal being to set check_untyped_defs = True in the config for the entire repo.

Also I was getting an error because the pinned version of jsonschema doesn't work with python 3.11 when I run tests on my laptop (it seems to work OK in CI/CD for some reason). I went ahead and updated all dev deps to latest possible version to get past this.

Also ran pyupgrade to remove old compatibility code since we now require python3.8+

@dekim24 dekim24 requested a review from a team as a code owner July 21, 2023 07:16
@dekim24 dekim24 requested a review from auramix July 21, 2023 07:16
.gitignore Outdated
.mypy_cache

#swagger_ui
flask_rebar/swagger_ui
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this directory hasn't been updated in 2 years, but it has a lot of large files that result in noisy matches when you search in the repo (I use ag to search). wondering if we can .gitignore it and someone who needs to actively update this directory can remove on a case by case basis?

Copy link
Contributor

Choose a reason for hiding this comment

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

That directory contains the minimized swagger ui files that we vendor with flask-rebar. It actually is being updated right now in this PR to support Open API 3.1

We should not gitignore a file because it's causing noisy matches when searching the file systems. That is the wrong use of the gitignore file.

Your file system searching tool of choice should have a method of excluding directories from the search. In the case of ag it looks like adding this pattern to a .ignore file should hopefully resolve your problem. 😄

@@ -1,4 +1,5 @@
from collections import namedtuple
from typing import Type
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this import is required for python <= 3.8, 3.9 and later support type[]

Copy link
Contributor

@airstandley airstandley left a comment

Choose a reason for hiding this comment

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

Looks great!
I have some minor nitpicks that I'd love to see addressed.

.gitignore Outdated
.mypy_cache

#swagger_ui
flask_rebar/swagger_ui
Copy link
Contributor

Choose a reason for hiding this comment

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

That directory contains the minimized swagger ui files that we vendor with flask-rebar. It actually is being updated right now in this PR to support Open API 3.1

We should not gitignore a file because it's causing noisy matches when searching the file systems. That is the wrong use of the gitignore file.

Your file system searching tool of choice should have a method of excluding directories from the search. In the case of ag it looks like adding this pattern to a .ignore file should hopefully resolve your problem. 😄

@dekim24 dekim24 requested a review from airstandley August 1, 2023 09:20
@dekim24 dekim24 merged commit 9dcd5e4 into master Aug 1, 2023
@dekim24 dekim24 deleted the ekim/mypy-2 branch August 1, 2023 20:50
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.

4 participants