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

Use mirrors-mypy for pre-commit #1291

Merged

Conversation

nathanjmcdougall
Copy link
Contributor

@nathanjmcdougall nathanjmcdougall commented Aug 7, 2023

I was having issues with the local hook for mypy.

Specifically, was getting inconsistent results between running pre-commit run --all-files versus pre-commit install and then git commit etc.

I changed this configuration to use https://github.com/pre-commit/mirrors-mypy, and once ensuring compliance across the codebase, I could get consistent results. I'm not sure of the motivation for using a local hook - if we need to stick with it, then I think there might need to be some more configuration to get ensure consistency.

For calls to pd.DataFrame.drop, I've preferentially used the columns argument since in some cases this was causing mypy issues.

Some formatting changes via black.

I also had some trouble considering how to test mypy compliance against different versions of python without creating separate environments for each version and switching between them. I don't know if you have any advice about this for contributors?

prefer df.drop(columns=...) syntax
remove cautious 3.6 import for Literal

Signed-off-by: Nathan McDougall <nmcdougall@tonkintaylor.co.nz>
Signed-off-by: Nathan McDougall <nmcdougall@tonkintaylor.co.nz>
black via pre-commit

Signed-off-by: Nathan McDougall <nmcdougall@tonkintaylor.co.nz>
black formatting

Signed-off-by: Nathan McDougall <nmcdougall@tonkintaylor.co.nz>
@codecov
Copy link

codecov bot commented Aug 7, 2023

Codecov Report

Patch coverage: 95.45% and project coverage change: +0.02% 🎉

Comparison is base (c1f9863) 93.82% compared to head (155fe49) 93.84%.
Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1291      +/-   ##
==========================================
+ Coverage   93.82%   93.84%   +0.02%     
==========================================
  Files          90       90              
  Lines        6704     6698       -6     
==========================================
- Hits         6290     6286       -4     
+ Misses        414      412       -2     
Files Changed Coverage Δ
pandera/engines/pandas_engine.py 95.42% <75.00%> (-0.02%) ⬇️
pandera/api/pandas/model.py 93.15% <100.00%> (+0.65%) ⬆️
pandera/api/pandas/types.py 96.00% <100.00%> (-0.16%) ⬇️
pandera/api/pyspark/types.py 100.00% <100.00%> (ø)
pandera/backends/pandas/checks.py 93.04% <100.00%> (ø)
pandera/backends/pyspark/container.py 67.47% <100.00%> (ø)
pandera/dtypes.py 95.49% <100.00%> (-0.04%) ⬇️
pandera/engines/numpy_engine.py 100.00% <100.00%> (ø)
pandera/strategies/pandas_strategies.py 97.82% <100.00%> (+<0.01%) ⬆️
pandera/typing/formats.py 100.00% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@nathanjmcdougall nathanjmcdougall marked this pull request as ready for review August 7, 2023 15:37
@@ -357,7 +357,7 @@ def strict_filter_columns(
)

if schema.strict == "filter":
check_obj = check_obj.drop(*filter_out_columns)
check_obj = check_obj.drop(columns=filter_out_columns)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I should have been more careful regarding pyspark.

Signed-off-by: Nathan McDougall <nmcdougall@tonkintaylor.co.nz>
Signed-off-by: Nathan McDougall <nmcdougall@tonkintaylor.co.nz>
@cosmicBboy cosmicBboy merged commit 85cdcec into unionai-oss:main Aug 22, 2023
40 checks passed
@cosmicBboy
Copy link
Collaborator

@nathanjmcdougall it seems like merging this PR is breaking linters on main... does this mypy error look familiar? https://github.com/unionai-oss/pandera/actions/runs/5942557462/job/16115774607

@nathanjmcdougall
Copy link
Contributor Author

I can't say precisely why that happened - presumably this branch was a few commits behind main in a way that tripped up mypy. I've made a fix branch at #1321.

@cosmicBboy
Copy link
Collaborator

Amazing, thanks!

@cosmicBboy cosmicBboy mentioned this pull request Jan 24, 2024
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.

2 participants