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

Fix deprecation warning on regex #654

Merged
merged 2 commits into from
Feb 26, 2021
Merged

Conversation

RubenRubens
Copy link
Contributor

This simple fix removes a warning in pytest.

@codecov
Copy link

codecov bot commented Feb 23, 2021

Codecov Report

Merging #654 (112024d) into master (2e8d674) will increase coverage by 0.19%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #654      +/-   ##
==========================================
+ Coverage   93.84%   94.03%   +0.19%     
==========================================
  Files          31       31              
  Lines        6447     6636     +189     
  Branches      691      726      +35     
==========================================
+ Hits         6050     6240     +190     
  Misses        258      258              
+ Partials      139      138       -1     
Impacted Files Coverage Δ
cadquery/selectors.py 98.01% <ø> (ø)
cadquery/occ_impl/shapes.py 91.89% <0.00%> (-0.03%) ⬇️
tests/test_cadquery.py 99.13% <0.00%> (+0.08%) ⬆️
cadquery/occ_impl/importers.py 88.42% <0.00%> (+0.09%) ⬆️
cadquery/cq.py 89.89% <0.00%> (+0.51%) ⬆️

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 2e8d674...112024d. Read the comment docs.

@marcus7070
Copy link
Member

These characters now get rendered as a link to something 🤷 in the HTML docs. https://cadquery--654.org.readthedocs.build/en/654/classreference.html#cadquery.selectors.ParallelDirSelector

Does pytest still throw warnings if you keep the raw string but add the backslash back in?

@RubenRubens
Copy link
Contributor Author

AFAIK r"\|(X|Y|Z)" would be displayed as \|(X|Y|Z). So, I don't find it ideal neither. 😕 Nice catch by the way. Should we open an issue on sphinx-doc/sphinx?

Sphinx decided it was a link to something
@marcus7070
Copy link
Member

marcus7070 commented Feb 25, 2021

I agree, the best solution would be to tell Sphinx that the pipe symbol is not... whatever it thinks it is (variable substitution? But why does that make a link?). I'm sure there is some way to do that, but sphinx is so complicated and I'll take the easy way out of by just escaping it.

Looks like r"\|(X|Y|Z)" is rendered correctly by sphinx in my local build, so I've added a commit.

@marcus7070
Copy link
Member

This looks good to me, I can't see any more weird links:

https://cadquery--654.org.readthedocs.build/en/654/classreference.html#cadquery.ParallelDirSelector

And the pytest warnings (which didn't show up on Azure, but did show up on Travis) are gone.

Feel free to try something different if you want to @RubenRubens and I'll have another look at it, or let me know if you want to merge as is.

@jmwright
Copy link
Member

All checks are green. Shall I merge?

@marcus7070
Copy link
Member

@jmwright, I'd just wait for @RubenRubens. If they are feeling like a deep dive into the sphinx internals, there might be a way to do this better. If they don't feel like a deep dive then this will do, I'm certainly not volunteering to figure out why sphinx thinks |X is a link to nowhere.

@RubenRubens
Copy link
Contributor Author

Great! Let's just merge this then.

@jmwright
Copy link
Member

Thanks @RubenRubens !

@jmwright jmwright merged commit c35b8ae into CadQuery:master Feb 26, 2021
@RubenRubens RubenRubens deleted the regex branch April 13, 2021 16:38
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