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

Add JSON RPC support from Rockets (resolve #284) #288

Merged
merged 1 commit into from
Jan 30, 2018

Conversation

tribal-tec
Copy link
Contributor

@tribal-tec tribal-tec commented Jan 26, 2018

  • all websocket messages are now JSON RPC
  • new RPC methods added: inspect, reset-camera, quit
  • schemas are exposed for RPC methods under v1/<method>/schema

@tribal-tec
Copy link
Contributor Author

ping

@tribal-tec
Copy link
Contributor Author

No comments? I would like to have a code review mainly. The approve I can get by just disabling the need for it :) I want you to know about the changes and the implications, also to avoid complaints about things that don't work anymore or differently.

@tribal-tec tribal-tec force-pushed the jsonrpc branch 2 times, most recently from 5fa5480 to b81c3c0 Compare January 29, 2018 18:51
@favreau
Copy link
Member

favreau commented Jan 29, 2018

Apologies for trusting your changes, that won't happen again ;-P I'll have a second look then.

@tribal-tec
Copy link
Contributor Author

Never trust anybody, especially me :) I have some cleanups to push still to tomorrow. Maybe you also find something. No worries though.

@favreau
Copy link
Member

favreau commented Jan 29, 2018

Sorry, I still do trust you! 🙂 But you're right, this needs proper review. Hopefully tomorrow.

@tribal-tec tribal-tec force-pushed the jsonrpc branch 2 times, most recently from 7e7256c to 5cd4197 Compare January 30, 2018 09:34
@@ -18,6 +18,21 @@
* 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
*/

#include "SDK.h"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would rename this file to somehting like json_serialization or whatever. SDK doesn't have the right meaning here.

}
}

// for rockets::jsonrpc
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there a reason to place these two functions separately, outside of the brayns namespace?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, to make it compile :) rockets expects them to be free functions within no namespace

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh ok, but then the same does not apply to the other from/to_json above..?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interestingly not... I move them together.

@@ -13,6 +13,10 @@ else()
list(APPEND EXCLUDE_FROM_TESTS braynsTestData.cpp)
endif()
if(NOT BRAYNS_OSPRAY_ENABLED)
list(APPEND EXCLUDE_FROM_TESTS brayns.cpp braynsTestData.cpp)
list(APPEND EXCLUDE_FROM_TESTS brayns.cpp braynsTestData.cpp SDK.cpp)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SDK renamed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch

* all websocket messages are now JSON RPC
* new RPC methods added: inspect, reset-camera, quit
* schemas are exposed for RPC methods under v1/<method>/schema
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants