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 support for ROS Service client #24

Closed
wants to merge 2 commits into from

Conversation

rgleichman
Copy link
Collaborator

This commit adds support for generating service messages and calling ROS services. This does not implement a ROS service provider. The most of the new code written after Services code review 1 are in pkgBuilder and depFinder.

parsed <- liftIO $ checkErrors <$> mapM parseMsg pkgMsgs
mapM_ (\(n, m) -> gen m >>= liftIO . B.writeFile n) (zip names parsed)
liftIO $ do f <- hasMsgs fname
parsedMsgs <- liftIO $ checkErrors <$> mapM parseMsg pkgMsgs
Copy link
Owner

Choose a reason for hiding this comment

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

This module isn't great from a focus perspective, and this definition typified the problem. It's my fault getting it to that state, but I think now there's enough pressure that it's worth splitting things out. We should probably have a helper for messages and a helper for services, maybe they could even be in a separate module.

If it's fresh in you're head, do try to take a whack at factoring it some. Admittedly, extracting pieces will be a bit of a pain as there are several things in scope at the point where things like genAndWriteService are defined, but I think it's worth trying to see if things can be made more clear.

@acowley
Copy link
Owner

acowley commented Nov 4, 2014

Any thoughts on dealing with the Travis build failures? Ideally, Travis could still run the tests, but I'd be fine with only a subset of tests running on Travis while the rest require a bit more setup.

@rgleichman
Copy link
Collaborator Author

Yes, I think Travis should only run a subset of the tests. This would mean it would run cabal test testexe instead of cabal test. Ideally, all of the tests (including service and topic tests) would be automated too.

@acowley
Copy link
Owner

acowley commented Dec 2, 2014

Merged!

@acowley acowley closed this Dec 2, 2014
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.

None yet

2 participants