[WIP] PypeS Refactor & Documentation#268
Conversation
This was necessary to do because of character encoding differences in python 2 and 3. Since we were note defining an explicit encoding, we were getting odd "non-ascii-character" errors in python 2. See PEP 263 https://www.python.org/dev/peps/pep-0263/ for more info.
This attempts to clarify and optomize the behavior of the pypes module. The previous method (using exec to import * from a module) is considered to be extremly sinful and risky. This should achieve the same purpose while preventing duplicate imports and non-necessary namespace creation.
… second pass through at refactoring
|
I should note that while this seems to work well in manual testing (atleast on my mac), the automated tests of the PypeS package are lacking and will need to be improved before I'd feel comfortable merging. |
|
Thanks Rick, this is great! Aha, yes, I wrote PypeS back in the days, it's badly in need of a refactor. |
lantiga
left a comment
lantiga
left a comment
There was a problem hiding this comment.
This looks really good to me, I didn't see anything raising a flag.
Just one thing I'm not sure you tried is argument pushing. You can force an input argument to be pushed through the pipe by appending @ to the argument, like
vmtksurfacenormals -ifile foo.vtp -normalsarray@ FooNormals --pipe ...
At this point the argument NormalsArrayName of type string will be seen by the following scripts.
I saw you didn't touch those lines, but it's worth documenting in the code.
| if '--nolog' in pypeArguments: | ||
| self.LogOn = 0 | ||
| if '--query' in pypeArguments: | ||
| if '--query' in pypeArguments: #TODO: What on earth does this do? |
There was a problem hiding this comment.
If I remember well 😄 , it was a facility to get all scripts from a package. Mainly done for PypePad. It's broken now, it should go for good.
|
As we think about moving this stuff into Python, we should figure out what do to with the |
|
Thanks luca, I have been testing the pushed arguments (that took me a while to figure out how it was working in the code, I'll definitely document it), and everything seems to work as expected. The one thing which is really bothering me is how method names are duplicated in Looking forward to our discussion, I definitely have a better handle on how you're thinking this will work. |
|
Yeah, so the reasoning is that Pype is the composition of one or more PypeScripts. So e.g. printing or parsing is something that the parent (Pype) has to do to parse the pype-wide args and break up the whole pype into calls to scripts, and then each PypeScript knows how to parse, etc its own script-specific option. |
|
Well that certainly makes sense, but there's got to be a simpler way to handle that. I'm sketching out a few ideas now for a refactor, and will propose them as I work through them. I'm really not a fan, since it's not clear at all where a message is being printed from (unless you have intimate knowledge of how the pypes system works). |
|
Since this will be a fairly large project, I'm going to create a seperate develop branch in the vmtk repo to direct all pull requests towards. It'll be easier to parse in the future if all this work (plus the work to come) is not performed in one giant github PR/branch. This is now going into the |
|
going to merge this into the |
2 participants
Hey @lantiga, in preparation for trying to integrate pype execution into jupyter notebooks, I started to really dig into the PypeS package. As I read through it, it was very apparent that the code was in need of a bit of TLC (most of it hasn't changed since you switched over from svn to git 11 years ago). This PR is the work that I've done just to try and understand/simplify what is happening when a pype is executed.
I originally just wanted to add docstrings and tests, but the complexity of the PypeS execution model meant I spent a lot of time working with the code just to understand what was going on. This turned into a bit more of a refactoring project that I intended. So far I've tried to (and hopefully succeeded at) keeping the class boundaries and core execution structure intact. Mostly I've just worked to simplify expressions, remove redundant or unused code, and extract individual units of work from large method blocks into their own functions.
It's a work in progress, and not really ready for a thorough review, but I wanted to put this up here so I could put it on your radar and ask a few questions along the way.