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

Added python lib wrapping the HTTP REST interface #26

Merged
merged 2 commits into from
Aug 17, 2016

Conversation

favreau
Copy link
Member

@favreau favreau commented Aug 12, 2016

No description provided.

@rdumusc
Copy link

rdumusc commented Aug 15, 2016

Looks very good in general!
A few open questions / remarks:

  • In TransferFunction class, how about renaming attributes -> channel?
  • I'm not a Python specialist, but for certain classes like Material, are the setters useful or could the member variables simply be public? Or is it designed so that the members look write-only..? I'm not super used to setters not beginning with set_* so I tend to confuse them with getters.
  • Add unit tests for the serialization of base classes?

@favreau
Copy link
Member Author

favreau commented Aug 16, 2016

Thanks for your comments:

  • Attribute vs channel was a long discussion already ;-) And channel was indeed one of the options, but we decided at the time that it was too RGBA related, and Brayns showed that other 'attributes' such as light emission are also part of the transfer function. So attribute was the winner of the long and passionate discussion :-)
  • I was myself not quite confident with the implementation, and I actually started with a get_/set_ syntax but decided to go for an API that would be a setter if you specify an argument, and getter if no argument is specified. Since then, I had another look at the documentation and read a bit more about what the standards would be. In the end, Python3 appears to offer annotations such as @Property that I think I should be using. I will therefore try this implementation and update the pull request.
  • Yep, unit testing... shame on me. Working on it :-)

@rdumusc
Copy link

rdumusc commented Aug 16, 2016

Arghh, too bad that Attribute won the debate ;-) Channel is more accurate and not tied to RGBA IMHO.
Good for the rest!

@hernando
Copy link
Contributor

Another candidate that I liked more was target.

@favreau
Copy link
Member Author

favreau commented Aug 16, 2016

I really liked channel as well, but unless we really want to go for another poll, I'll stick to attribute.

@favreau favreau force-pushed the master branch 2 times, most recently from e9ba468 to 153bb83 Compare August 16, 2016 13:12
@rdumusc
Copy link

rdumusc commented Aug 16, 2016

+1

@favreau
Copy link
Member Author

favreau commented Aug 17, 2016

retest this please

@favreau favreau merged commit fb65994 into BlueBrain:master Aug 17, 2016
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.

3 participants