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 in CI [WIP] #378

Merged
merged 22 commits into from
Jun 13, 2020
Merged

MyPy in CI [WIP] #378

merged 22 commits into from
Jun 13, 2020

Conversation

adam-urbanczyk
Copy link
Member

@adam-urbanczyk adam-urbanczyk commented Jun 9, 2020

Just running mypy over the codebase. There are no annotations yet.

EDIT:
I added some annotations in geom.

@codecov
Copy link

codecov bot commented Jun 9, 2020

Codecov Report

Merging #378 into master will decrease coverage by 2.18%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #378      +/-   ##
==========================================
- Coverage   95.50%   93.31%   -2.19%     
==========================================
  Files          19       19              
  Lines        4805     4847      +42     
  Branches        0      483     +483     
==========================================
- Hits         4589     4523      -66     
+ Misses        216      210       -6     
- Partials        0      114     +114     
Impacted Files Coverage Δ
cadquery/occ_impl/exporters.py 96.44% <100.00%> (-0.59%) ⬇️
cadquery/occ_impl/geom.py 87.71% <100.00%> (-0.92%) ⬇️
cadquery/cq.py 87.41% <0.00%> (-6.68%) ⬇️
tests/__init__.py 87.50% <0.00%> (-6.25%) ⬇️
cadquery/cqgi.py 81.01% <0.00%> (-5.07%) ⬇️
cadquery/occ_impl/importers.py 86.74% <0.00%> (-4.82%) ⬇️
tests/test_importers.py 92.00% <0.00%> (-4.00%) ⬇️
cadquery/occ_impl/shapes.py 90.15% <0.00%> (-1.67%) ⬇️
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e55b7ed...87ea98d. Read the comment docs.

@adam-urbanczyk adam-urbanczyk requested a review from jmwright June 10, 2020 15:53
@adam-urbanczyk
Copy link
Member Author

@jmwright could you take a look at the changes in geom? I like having type annotations in the code, but I'm curious what other people think.

Copy link
Member

@jmwright jmwright left a comment

Choose a reason for hiding this comment

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

@adam-urbanczyk Looks good, but I'm curious what prompted the interest in static typing. There is probably an issue somewhere that led to this, but I can't recall.

cadquery/occ_impl/geom.py Outdated Show resolved Hide resolved
@adam-urbanczyk
Copy link
Member Author

adam-urbanczyk commented Jun 10, 2020

#244 (which is now split, so #377 ) and #247 . I strongly believe that we are reaching a point where "static" typing will be helpful. Especially together with something like pyls-mypy

@adam-urbanczyk
Copy link
Member Author

BTW: I'm also experimenting with generating stubs for OCP.

@jmwright
Copy link
Member

I strongly believe that we are reaching a point where "static" typing will be helpful.

Ok, +1 for merging this once the checks pass.

BTW: I'm also experimenting with generating stubs for OCP.

Sounds good. Is your work viewable anywhere, or are you just experimenting on your local machine?

@adam-urbanczyk
Copy link
Member Author

Not yet, though the modified tool to make them is pushed: https://github.com/CadQuery/pybind11-stubgen . There are still some fine points related to deploying them.

@adam-urbanczyk
Copy link
Member Author

@jmwright I added the OCP stubs here: https://github.com/CadQuery/OCP-stubs . It was necessary to make this thing work. I did manage to find one bug in the geom.py code that way, I think this will be very helpful in the long run. Do you want to take another look, or should I merge as is?

@jmwright
Copy link
Member

I'm good with merging.

@adam-urbanczyk
Copy link
Member Author

OK, if this round passes (black was complaining) I'll merge

@adam-urbanczyk adam-urbanczyk merged commit f38f9cb into master Jun 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants