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

Improve Inheriting Workplane Fluent Methods #677

Merged
merged 6 commits into from
Mar 12, 2021

Conversation

pavpen
Copy link
Contributor

@pavpen pavpen commented Mar 5, 2021

  • Updated Workplane.workplane(), Workplane.copyWorkplane(), Workplane.newObject() to return instances of a type that's based on the input parameters (such as self).
  • Updated type hints to reflect that return types of fluent methods depend on the type of method arguments.
  • Added a unit test to verify fluent methods in a derived class return instances of the derived class.

This is a pull request for issue #673.

* Updated `Workplane.workplane()`, `Workplane.copyWorkplane()`, `Workplane.newObject()` to return instances of a type that's based on the input parameters (such as `self`).
* Updated type hints to reflect that return types of fluent methods depend on the type of method arguments.
* Added a unit test to verify fluent methods in a derived class return instances of the derived class.
@codecov
Copy link

codecov bot commented Mar 5, 2021

Codecov Report

Merging #677 (9534a4c) into master (0325474) will increase coverage by 0.08%.
The diff coverage is 94.23%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #677      +/-   ##
==========================================
+ Coverage   94.19%   94.28%   +0.08%     
==========================================
  Files          31       31              
  Lines        6689     6752      +63     
  Branches      725      727       +2     
==========================================
+ Hits         6301     6366      +65     
+ Misses        253      252       -1     
+ Partials      135      134       -1     
Impacted Files Coverage Δ
cadquery/cq.py 91.05% <93.33%> (+0.24%) ⬆️
tests/test_cadquery.py 99.12% <100.00%> (+0.02%) ⬆️
cadquery/occ_impl/shapes.py 91.95% <0.00%> (+0.03%) ⬆️

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 0325474...9534a4c. Read the comment docs.

* Fixed type hints in `sweep()` and `_selectObjects()`.
* Ran `black` on `test_cadquery.py`.
@marcus7070
Copy link
Member

AppVeyor is failing a single test on 3.6, testOffset2D:

self.assertEqual(s.solids().size(), 4)
E       AssertionError: 1 != 4
tests\test_cadquery.py:4093: AssertionError

That test passes in all other CI envs, including Win 3.6 on Azure. @adam-urbanczyk this looks super weird, you might want to re-run that test? I don't have the permissions.

Although honestly, if it goes away after re-running that's going to be just as confusing. It's not like it's a network error...

@pavpen
Copy link
Contributor Author

pavpen commented Mar 9, 2021

Hm. Yeah, these tests pass for me with Python 3.6.13 and 3.9.2 on Windows. It would be useful if someone is able to reproduce this error.

I can also push a whitespace change to force a re-run of the appveyor job. Or, is there a way to re-run it manually?

@marcus7070
Copy link
Member

@adam-urbanczyk should have the permissions to re-run it manually, but he's busy at the moment. so feel free to do the whitespace change. Here is a (hopefully semi-permanent) link to the failing job for posterity: https://ci.appveyor.com/project/adam-urbanczyk/cadquery-icqj5/builds/38091458/job/k9pg21fnvqup9be5

@adam-urbanczyk
Copy link
Member

If all fails, you can always close an reopen to trigger CI

@adam-urbanczyk adam-urbanczyk self-requested a review March 9, 2021 07:36
Copy link
Member

@adam-urbanczyk adam-urbanczyk left a comment

Choose a reason for hiding this comment

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

One change request, otherwise looks good!

cadquery/cq.py Outdated Show resolved Hide resolved
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.

I think once the requested change is done this will be ready to merge. Thanks @pavpen !

* Renamed `WorkplaneT` to `T` in `cq.py`.
* Re-formatted `cq.py` with `black`.
* Re-formatted `cq.py` with `black` 19.10b0.
@adam-urbanczyk
Copy link
Member

Thanks @pavpen !

@adam-urbanczyk adam-urbanczyk merged commit a54de64 into CadQuery:master Mar 12, 2021
@pavpen
Copy link
Contributor Author

pavpen commented Mar 12, 2021

Thanks for the help! If you can figure out the differences between execution environments, it could help track the causes of the flaky unit tests.

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