Skip to content

Conversation

@Peque
Copy link
Contributor

@Peque Peque commented Dec 22, 2019

No description provided.

@codecov
Copy link

codecov bot commented Dec 22, 2019

Codecov Report

Merging #255 into master will increase coverage by <.01%.
The diff coverage is 94.07%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #255      +/-   ##
==========================================
+ Coverage   94.91%   94.91%   +<.01%     
==========================================
  Files          18       18              
  Lines        4226     4229       +3     
==========================================
+ Hits         4011     4014       +3     
  Misses        215      215
Impacted Files Coverage Δ
tests/test_selectors.py 100% <100%> (ø) ⬆️
tests/test_cqgi.py 100% <100%> (ø) ⬆️
tests/test_cad_objects.py 99.33% <100%> (ø) ⬆️
cadquery/occ_impl/importers.py 96.29% <100%> (ø) ⬆️
tests/test_jupyter.py 100% <100%> (ø) ⬆️
cadquery/occ_impl/jupyter_tools.py 100% <100%> (ø) ⬆️
cadquery/__init__.py 100% <100%> (ø) ⬆️
cadquery/selectors.py 99.09% <100%> (ø) ⬆️
tests/test_importers.py 93.93% <100%> (+0.18%) ⬆️
tests/test_exporters.py 100% <100%> (ø) ⬆️
... and 8 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 74573fc...b1f6b8c. Read the comment docs.

@Peque Peque marked this pull request as ready for review December 22, 2019 20:10
@jmwright
Copy link
Member

@Peque With this check added to CI, if the lint check fails, the user will need to run black on their local copy of the code, correct? We'll need to document that for contributors.

@Peque
Copy link
Contributor Author

Peque commented Dec 27, 2019

@jmwright Good idea! 😊

I added this in the README.md file: 6e73015

Also, rebased against upstream after the latest changes by Bruno.

@Peque
Copy link
Contributor Author

Peque commented Dec 27, 2019

@jmwright I could also add a recommendation to use --fixup when commiting changes to the pull request after review (like I did here for example), to then finally autosquash everything before merging the changes. But maybe that is not what you are used to...

@jmwright
Copy link
Member

@Peque Yes, that's a very different workflow from what I'm used to. I don't know enough to really understand what that would do to the commit tree.

@Peque
Copy link
Contributor Author

Peque commented Dec 27, 2019

Maybe I should also add that adding assertions to the test is a must?

It seems the latest changes integrated (in particular the test_assembleEdges test in test_cadquery.py) do not have any assertions.

@Peque
Copy link
Contributor Author

Peque commented Dec 27, 2019

@Peque
Copy link
Contributor Author

Peque commented Dec 27, 2019

@jmwright Updated the guidelines with: 34db27c

@Peque
Copy link
Contributor Author

Peque commented Dec 29, 2019

@adam-urbanczyk Any thoughts on this? 😊

@adam-urbanczyk
Copy link
Member

@Peque cf. my comment above. Otherwise I am wondering if it would be possible to add a pre-commit hook to streamline using black. Do you happen to have any experience in doing that? This https://pre-commit.com/ looks like a relevant solution.

@Peque
Copy link
Contributor Author

Peque commented Dec 30, 2019

@adam-urbanczyk Rebased and force-pushed. Emoji were removed. 💔

No experience with pre-commit hooks, sorry. I personally don't like that very much. I am more in favor of: your computer, your rules. You decide when to run those checks and how. As long as your changes pass the pipelines, you are fine. Maybe if I spent some time trying them I would change my mind, but I have not been encouraged too much to try them so far.

@Peque
Copy link
Contributor Author

Peque commented Jan 20, 2020

@adam-urbanczyk Friendly ping. 😊

@adam-urbanczyk
Copy link
Member

Looks good @Peque , @jmwright are you OK with merging this?

@jmwright
Copy link
Member

@adam-urbanczyk Yes, +1

@adam-urbanczyk adam-urbanczyk merged commit 102c16c into CadQuery:master Jan 20, 2020
@adam-urbanczyk
Copy link
Member

Thanks again for the contribution @Peque !

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.

3 participants