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

Raise exception if appropriate bson module is installed. #270

Merged
merged 2 commits into from
Nov 20, 2017

Conversation

ledmonster
Copy link
Contributor

Check whether mongodb oriented bson is installed or not just after init_node,
and if appropriate bson is not installed, shutdown node with error message.

See: #198

@cjds
Copy link

cjds commented Mar 10, 2017

This wrong BSON library thing tripped me up for quite a while. I really appreciate the PR

@jihoonl jihoonl self-assigned this May 12, 2017
@jihoonl
Copy link
Member

jihoonl commented Jun 28, 2017

ping

rospy.logerr("BSON installation does not support all necessary features. "
"Please use the MongoDB BSON implementation.")
rospy.signal_shutdown("shutdown")

Copy link
Member

Choose a reason for hiding this comment

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

It should not complain if the node is default mode(b64).

Can you add this error only when either bson_only_mode is True or [binary_encoder_type](https://github.com/RobotWebTools/rosbridge_suite/blob/develop/rosbridge_library/src/rosbridge_library/internal/message_conversion.py#L73 is bson?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for your review. I'll fix it.

if not appropriate_bson_installed():
rospy.logerr("BSON installation does not support all necessary features. "
"Please use the MongoDB BSON implementation.")
rospy.signal_shutdown("shutdown")
Copy link
Member

Choose a reason for hiding this comment

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

Same as above

rospy.logerr("BSON installation does not support all necessary features. "
"Please use the MongoDB BSON implementation.")
rospy.signal_shutdown("shutdown")

Copy link
Member

Choose a reason for hiding this comment

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

Here too.

@jihoonl
Copy link
Member

jihoonl commented Aug 30, 2017

@ledmonster sorry I just realized that all reviews were in pending. Could you change the requested in review?

…node. (RobotWebTools#198)

Check whether mongodb oriented bson is installed or not just after init_node,
and if appropriate bson is not installed, shutdown node with error message.

See: RobotWebTools#198
Copy link
Contributor Author

@ledmonster ledmonster left a comment

Choose a reason for hiding this comment

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

@jihoonl I have totally changed the code. Could you please review again?

As to bson_only_mode or binary_encoder_type, even if binary_encoder_type is not bson, bson is imported and used in many places. So I think it's better to always check bson.

rospy.logerr("BSON installation does not support all necessary features. "
"Please use the MongoDB BSON implementation.")
rospy.signal_shutdown("shutdown")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for your review. I'll fix it.

@jihoonl
Copy link
Member

jihoonl commented Nov 20, 2017

Looks good to me. thanks. merging it now.

@jihoonl jihoonl merged commit a2f0149 into RobotWebTools:develop Nov 20, 2017
@jihoonl jihoonl changed the title Check if appropriate bson module is installed or not just after init_node (#198) Raise exception if appropriate bson module is installed. Nov 20, 2017
Behery pushed a commit to Behery/rosbridge_suite that referenced this pull request Jan 9, 2018
…s#198) (RobotWebTools#270)

* Raise Exception if inappropriate bson module is installed (Related to RobotWebTools#198)
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