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

Run mypy on all files #1815

Merged
merged 2 commits into from Apr 20, 2018

Conversation

Projects
None yet
2 participants
@toolness
Contributor

toolness commented Apr 20, 2018

This fixes #1803 by upgrading mypy to its latest version, slightly reducing its strictness, running it on all files, and fixing the errors that came up.

Notes

  • The only loosening of mypy's configuration is that disallow_untyped_calls is now False instead of True.

  • We don't run mypy on tests or migrations right now.

  • I ended up just getting rid of all our django mypy stubs, because they were severely limiting our ability to run mypy on all our files: as far as I can tell, stubs can't just define a subset (or submodule) of a package, they need to define everything, which meant that mypy was complaining that a bunch of the Django APIs our code used didn't exist, since we didn't define everything in our stub. So it was easier to just remove the stub entirely.

    I think this will make our code more maintainable going forward too: had I actually appeased mypy here, we'd continuously run into problems going forward, as mypy would get upset at every new Django API we decided to use.

  • I am not a big fan of how mypy complains about assignments to empty lists or dicts as being untyped. I wish there were a mypy configuration setting to just treat untyped dicts/lists as Dict[Any, Any] or List[Any], respectively, but as far as I can tell, there isn't. It's particularly annoying because it forces us to do a from typing import List, Dict, Any at the top of every file that needs it.

@@ -37,4 +37,4 @@ class Meta:
'hourly_rate_year1', 'current_price', 'next_year_price',
'second_year_price', 'schedule', 'sin', 'contractor_site',
'business_size')
list_serializer_class = ContractSerializer
list_serializer_class = ContractListSerializer

This comment has been minimized.

@toolness

toolness Apr 20, 2018

Contributor

Oof, we were actually defining ContractSerializer twice in the same file! Good catch, mypy.

sql = ( # nosec
"UPDATE contracts_contract "
" SET _normalized_labor_category = v.nlc"
" FROM (VALUES" + values + ") AS v (id, nlc)"
" FROM (VALUES" + values_str + ") AS v (id, nlc)"

This comment has been minimized.

@toolness

toolness Apr 20, 2018

Contributor

Mypy doesn't like it when a variable changes its type mid-code, so I changed the variable name here to appease it.

return decorator

This comment has been minimized.

@toolness

toolness Apr 20, 2018

Contributor

Aside from working around python/mypy#2087, this also makes assigning the short description easier to read.

@@ -79,7 +87,8 @@ def smart_load_from_upload(classname, f):
pass
if pricelist is None:
raise original_error
default_error = ValidationError('Unrecognized price list!')
raise original_error or default_error

This comment has been minimized.

@toolness

toolness Apr 20, 2018

Contributor

I'm not really sure if default_error will ever be raised, but mypy was concerned about original_error possibly being None when it was raised (which is bad, because raised exceptions should always subclass BaseException). So I made default_error partly to appease it and partly to cover our backs. (That said, it's not a great sign when it's hard for both a human and mypy to infer whether original_error might be None here, but I wanted to limit the scope of this PR.)

@@ -95,6 +95,8 @@
# If the Docker host is running on Windows, we don't need this
# module, so it's OK to not import it.
import pwd
else:
pwd = None

This comment has been minimized.

@toolness

toolness Apr 20, 2018

Contributor

Bleh. I wanted to make sure mypy ran without errors on both Windows and Linux; running it on Windows complained that pwd doesn't exist when it's used, so I added some logic to appease mypy on Windows. I think it is strictly appeasement though, since the code that actually uses pwd will only ever be run in the docker container (which uses Linux).

This comment has been minimized.

@jseppi

jseppi Apr 20, 2018

Contributor

all those if pwd checks are kinda yucky. Could mypy checking just be disabled for pwd within this file? I'm not totally sure of all the escape hatches mypy has, so maybe that isn't possible.

This comment has been minimized.

@toolness

toolness Apr 20, 2018

Contributor

Yeah, unfortunately disabling type checking for pwd is what I originally tried, which satisfied the windows side, but then the linux side was like "yo dawg u don't need to disable type checking here" which made mypy fail. Blugh it is annoying. So I think I am just gonna leave it in this annoying state for now.

match = self.regex.search(contents)
if match is None:
raise ValueError(f"{self.name} not found in {filename}!")
return match

This comment has been minimized.

@toolness

toolness Apr 20, 2018

Contributor

Decided to do this whole rigmarole because we were just calling search on a regexp without checking to see if it returned None. Mypy rightfully complained about this, so I made this little abstraction to make any unsuccessful searches raise helpful errors.

@toolness toolness requested a review from jseppi Apr 20, 2018

@jseppi

jseppi approved these changes Apr 20, 2018

🎉 Sweet!

@@ -95,6 +95,8 @@
# If the Docker host is running on Windows, we don't need this
# module, so it's OK to not import it.
import pwd
else:
pwd = None

This comment has been minimized.

@jseppi

jseppi Apr 20, 2018

Contributor

all those if pwd checks are kinda yucky. Could mypy checking just be disabled for pwd within this file? I'm not totally sure of all the escape hatches mypy has, so maybe that isn't possible.

@toolness toolness merged commit dbe1a13 into develop Apr 20, 2018

3 checks passed

ci/circleci: build Your tests passed on CircleCI!
Details
codeclimate Approved by Atul Varma.
Details
codeclimate/total-coverage 92% (0.0% change)
Details

@toolness toolness deleted the mypy-all-the-things branch Apr 20, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment