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

Extendend string selector syntax using PyParsing #148

Merged
merged 8 commits into from
Jun 30, 2016

Conversation

adam-urbanczyk
Copy link
Contributor

Do not merge yet - WIP

So far only the grammar is implemented. It extends the current one with:

  • python style indexing: e.g. >Z[10]
  • arbitrary vectors: e.g. +(1.,1.,0)

@coveralls
Copy link

Coverage Status

Coverage increased (+0.2%) to 85.799% when pulling f8c377c on adam-urbanczyk:master into 0ef569a on dcowden:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.2%) to 85.799% when pulling f8c377c on adam-urbanczyk:master into 0ef569a on dcowden:master.

@hyOzd
Copy link
Contributor

hyOzd commented Jun 21, 2016

Hi @adam-urbanczyk , do you plan to make selector combining a part of this new syntax?

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.1%) to 85.509% when pulling 76a2207 on adam-urbanczyk:master into 0ef569a on dcowden:master.

Fixed all failing test cases from the previous commit: extended the
grammat to handle upper and lowercase CQ types and fixed some typos
@coveralls
Copy link

Coverage Status

Coverage increased (+0.3%) to 85.884% when pulling d1593e6 on adam-urbanczyk:master into 0ef569a on dcowden:master.

@adam-urbanczyk
Copy link
Contributor Author

@hyOzd not in the scope of this PR. Maybe in a following one, shouldn't be that hard

@dcowden I think this is almost it - all tests pass. Please review and tell me what you think. BTW: I still want to add more test cases.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.4%) to 85.99% when pulling 5b0b5c0 on adam-urbanczyk:master into 0ef569a on dcowden:master.

@adam-urbanczyk
Copy link
Contributor Author

@dcowden @jmwright I added additional test case (and fixed some parsing bugs that it pointed to). From my side it is ready to be merged.

@hyOzd @dcowden @jmwright do you think that adding logical ops to the string syntax is a good idea? If so I can start thinking about another PR

@dcowden
Copy link
Owner

dcowden commented Jun 29, 2016

This looks ok to me. Great work @adam-urbanczyk , nice contribution!
@jmwright if you are ok with this please merge, or let me know and i will

@jmwright
Copy link
Collaborator

@adam-urbanczyk Great work, thank you.

@adam-urbanczyk @dcowden How do we want to handle the new dependency (pyparse)? Should it be included directly with CadQuery, or should we just add instructions on how to install it in the docs? I think Windows users might have to jump through a few hoops to install that package.

My vote would be to add logical ops. They're so powerful that I think it would be worth the effort if you're willing to do it, @adam-urbanczyk

@dcowden
Copy link
Owner

dcowden commented Jun 29, 2016

yes, +1 on the logical ops.

on managing the dependency, that's a very good question. I did some quick prototyping a while back, and I concluded that packaging it with cadquery is not really a good idea. I extracted the source, and i found that non-trivial source code modifications were required to make it happy living in our package structure.

So unfortunately I think we'll have to require an external dependency. You're right that this is a pain, especially on windows.

But on another level, the installation process for CQ and all its deps is already completely out of control-- so i dont see a reason to hold this up due to one more.

Instead, we need to 'lean in' to the dependency problem and shift to delivery via containers and/or conda packages-- so that we can have all the deps we want and still not make it hard for the user.

@jmwright
Copy link
Collaborator

I'll try to get this merged tonight.

@hyOzd
Copy link
Contributor

hyOzd commented Jun 29, 2016

@adam-urbanczyk having logical ops as part of string selector would be awesome.

@jmwright
Copy link
Collaborator

Ready to merge. Thanks again for this contribution. I'm looking forward to putting this new selector syntax to use. I'll create a separate issue about documenting the new dependency so that it doesn't fall through the cracks.

@jmwright jmwright merged commit ffa54fb into dcowden:master Jun 30, 2016
@dcowden
Copy link
Owner

dcowden commented Jun 30, 2016

Thanks again @adam-urbanczyk for this work, and thanks @jmwright for merging!

@jmwright
Copy link
Collaborator

jmwright commented Jul 4, 2016

I updated the CadQuery module for FreeCAD for this change and embedded pyparsing as a library. I played around a bit with the new selector syntax and things seemed to work well.

@adam-urbanczyk
Copy link
Contributor Author

Good to hear. I am working on a semi-complicated model using Nth selector (which actually was the reason to develop it) . Once I am done, I can post it somwhere.

@jmwright
Copy link
Collaborator

jmwright commented Jul 4, 2016

We have a discussion thread on the Google Group for sharing work if you're interested in using that.

https://groups.google.com/forum/#!topic/cadquery/mS2ITd37sQA

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

5 participants