Skip to content
This repository has been archived by the owner on Feb 9, 2021. It is now read-only.

DirectionNthSelector --> selects Nth face/wire normal/parallel to given direction #147

Merged
merged 5 commits into from
Jun 8, 2016

Conversation

adam-urbanczyk
Copy link
Contributor

Based on my short experience with CQ, this feature is really helpful when working with complicated models. Please review and tell me what you think.

BTW: I'd be glad to implement a string shortcut for this selector too, but I'll need few pointers first.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.1%) to 85.628% when pulling 9df0e39 on adam-urbanczyk:master into a54a819 on dcowden:master.

@dcowden
Copy link
Owner

dcowden commented Jun 5, 2016

This looks like a great feature! thanks for contributing, and for adding a test also.

Adding a string version of this selector would require modifying _chooseSelector(self,selType,selAxis) inside of StringSyntaxSelector, and then modifying doc/selectors.rst to include the syntax. I'm not sure I can think of an easy way for this to be a string selector, since it takes a parameter too.

If you have a suggestion for the syntax, i'm happy to help you implement it. Otherwise, I think this is good as-is.

@dcowden
Copy link
Owner

dcowden commented Jun 5, 2016

BTW as a side note, after we build next, your selector should automatically appear in the docs, here:

http://dcowden.github.io/cadquery/classreference.html

@jmwright
Copy link
Collaborator

jmwright commented Jun 5, 2016

Wow, thanks for this! It's one of the features that I've thought a lot about since getting involved in CadQuery.

For min/max selectors along the same vein, I've wondered if a sting selector like "<Z2" would work to select the second face. So, would something like "|Z2" or "|<Z2" work for the string representation of this selector?

@dcowden
Copy link
Owner

dcowden commented Jun 5, 2016

Yeah, >Z2 could work. could we add a delimiter that could become standard for this type of thing? so like >Z.2 or |Z.2

That way, we could write standard code that first breaks apart the selector based on the first character, and then breaks apart the clauses using "." or "," or something.

In this case, i think we should actually deprecate the original ParallelDir selector. This one is a superset, with special value N=1.

@jmwright
Copy link
Collaborator

jmwright commented Jun 5, 2016

The delimiter sounds like a good idea. As you suggested, I think that would also deprecate the original parallel dir selector, and will probably lead to the same deprecation of the min/max selector eventually.

I'm not sure I have a preference on "." vs "," as the delimiter. The comma might clue users in a little better to the fact that the 2 is a parameter.

@adam-urbanczyk
Copy link
Contributor Author

Gents, what would you say to ">Z[2]", i.e. using python-like syntax for indexing?

On a related topic: I had a look at the _chooseSelector. Would rewriting it to use something else than regular expressions (say pyparsing module) be a welcome contribution? I think it would make adding more features easier (e.g. handling of ">(1,1,0)" ), if you are willing to accept additional dependencies.

Anyhow, if you are happy with the quality of the current PR please merge, and let's handle the string selector discussion in a different PR (I can work on it, but I will not promise any specific time-frame).

@jmwright
Copy link
Collaborator

jmwright commented Jun 6, 2016

I really like the indexing syntax. My vote is to go with that.

I'd like to hear from Dave about the additional dependency with adding pyparsing. We've been resisting adding external dependencies to this core library, but we're getting to the point where it's going to be unavoidable. Dave is already looking at a DXF import library.

This PR looks good to me. I'd say it's ready to merge.

@dcowden
Copy link
Owner

dcowden commented Jun 6, 2016

agreed. i'll merge the existing PR.

yes, Jeremy said it best on dependencies. For a while i was trying to avoid
them. but it is clear we need DXF import, and it is clear we want to
introduce a dependency for that. in fact, my memory may be wrong, but I
actually think that pyparsing specifically was a dep of the dxf lib i
wanted to use. So i think that direction is fine.

On Mon, Jun 6, 2016 at 2:12 PM, Jeremy Wright notifications@github.com
wrote:

I really like the indexing syntax. My vote is to go with that.

I'd like to hear from Dave about the additional dependency with adding
pyparsing. We've been resisting adding external dependencies to this core
library, but we're getting to the point where it's going to be unavoidable.
Dave is already looking at a DXF import library.

This PR looks good to me. I'd say it's ready to merge.


You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#147 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/ABPOA_VSukjIaMOQ0Uw5PxN8LbhlrhOtks5qJGMAgaJpZM4IubS1
.

@dcowden dcowden merged commit 0ef569a into dcowden:master Jun 8, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants