-
Notifications
You must be signed in to change notification settings - Fork 10
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
Enh/simplify node #178
Enh/simplify node #178
Conversation
Looks like a really good approach. I just have a few remarks, some are more minor:
|
Oh and I would keep the definition of new nodes out of this PR, i.e. the |
Thanks for the input! I followed your suggestion and created a unified |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
very nice. Just some minor points.
a0538c1
to
6faefd0
Compare
thanks for the input @mschmidt87! i've implemented some of your suggestions and improved the doc slightly. please have another look! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving with two (minor) requests:
- Short explanation of the magic number in the test (either in the function or in a new fixture).
- What about changing the title of the documentation about nodes to "Operator Nodes" now (instead of currently "Functional Nodes") since you introduced that class name in thisPR?
7c41f1d
to
49a799e
Compare
49a799e
to
f2c94b2
Compare
ok, squashed my commits and removed the magic number again, it doesn't make sense here, you were right! let's see if travis is happy, then we can merge 👍 |
This PR implements abstract classes that make the definition of new nodes much easier and more compact. New nodes, i.e., operators can now be introduced in two lines:
For operators that are "special", a few more lines are needed to define the different flavors for the
to_...
functions:or
Please have a look at the suggested implementation @mschmidt87 @HenrikMettler and let me know what you think. Is this approach sensible, user friendly, maintainable, flexible enough, etc pp?
Note that there still seems to be some duplication of code, however I am not sure how much further we can condense this while keeping the implementation (fairly) transparent.
closes #165