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

Services code review 1 #22

Closed
wants to merge 44 commits into from
Closed

Services code review 1 #22

wants to merge 44 commits into from

Conversation

@rgleichman
Copy link
Collaborator

@rgleichman rgleichman commented Oct 18, 2014

I have been working on implementing services and thought it would be nice to get some feedback. This is still a work in progress, so this should not be merged.

Check out the new tests in Tests/MsgGen.hs, and Tests/ServiceClientTests/ServiceClientTest.hs (instructions in comments). Right now it can generate Haskell types for service messages (tested by MsgGen), and call ROS services without any error handling (tested by ServiceClientTest).

Service message generation is not yet integrated into the executable.

Service Haskell types are just like message types except that they are also instances of SrvInfo. The generated Haskell message type files should be the same as before except they might have an extra newline at the end.

Except for MsgGen, changes to existing code are minimal: there is some code extraction and changes suggested by hlint.

rgleichman added 29 commits Jul 20, 2014
…n be generated simultaneously. The gen tests now pass, still need to update md5 specific tests.
…te (in progress), add add_two_ints_server.py as a local file for servicetest.
@rgleichman
Copy link
Collaborator Author

@rgleichman rgleichman commented Oct 18, 2014

The Travis build fails since ServiceClientTest (servicetest test suite) needs manual set up.

@rgleichman
Copy link
Collaborator Author

@rgleichman rgleichman commented Oct 23, 2014

Since making the pull request I have added error detection and exception handling to callService.

-- | The first argument is a path to the test directory
-- | The second argument is a list of paths to the message file
-- | The third argument is a list of paths to the golden Haskell file
-- | Precondition: cachePkg has been called

This comment has been minimized.

@acowley

acowley Oct 24, 2014
Owner

Haddock syntax doesn't need the vertical bar character (|) on each line.

Perhaps we should deal with the requirement to call cachePkg ourselves by keeping a Set of cached packages, and calling cachePkg ourselves when loading a message definition from an uncached package.

return $ testGroup "Generated Haskell" $ zipWith3
(\name gold gen -> testCase name $ gold @=? gen) msgPaths golds gens

-- | moduleNames is a list of the other messages in the package

This comment has been minimized.

@acowley

acowley Oct 24, 2014
Owner

Another minor note on comments. Here the argument name moduleNames is out of date, so it's not so helpful. In general, though, casual mention of argument names is a haddock trap as the argument names don't appear in the generated documentation. If you want to refer to argument names, it's good to start out by writing a representative invocation such as, @testGeneratedService packagePath srvPath requestGolden responseGolden pkgMsgs@ tests a service defined at @srvPath@ .... Note that, in haddock, the @ character is used to typeset text in a monospace font, while a single quote typesets and hyperlinks.

@@ -0,0 +1,41 @@
{-# LANGUAGE OverloadedStrings, DeriveDataTypeable, DeriveGeneric #-}

This comment has been minimized.

@acowley

acowley Oct 24, 2014
Owner

Should this and the generated Response be in the repository?

This comment has been minimized.

@rgleichman

rgleichman Oct 25, 2014
Author Collaborator

They are needed by ServiceClientTest.hs. I do not know what the alternative would be. It might be nice to have an integration test that both generates the message types and calls the service.

@@ -169,8 +174,21 @@ test-suite testexe
type: exitcode-stdio-1.0
hs-source-dirs: Tests, src/executable
main-is: AllTests.hs
ghc-options: -Wall
ghc-options: -Wall -O0

This comment has been minimized.

@acowley

acowley Oct 24, 2014
Owner

Curious about the -O0. Is it just for faster compilations?

This comment has been minimized.

@rgleichman

rgleichman Oct 24, 2014
Author Collaborator

Yes.

-- Fully-qualified name of service
-- Returns (int, str, str)
-- (code, statusMessage, serviceUrl)
-- service URL provides address and port of the service. Fails if there is no provider.

This comment has been minimized.

@acowley

acowley Oct 24, 2014
Owner

Can you add an example function call with the argument described? Something like,

@lookupService u s1 s2@ where @U@ is ...

(uriRegName u)
Nothing -> error $ "Couldn't parse URI "++target

parsePort :: URI -> Integer

This comment has been minimized.

@acowley

acowley Oct 24, 2014
Owner

Might as well return an Int or whatever integral type is expected by the call site below.

liftIO $ sendBS sock bytes
handle <- liftIO $ socketToHandle sock ReadMode
result <- getServiceResult handle
liftIO $ hClose handle

This comment has been minimized.

@acowley

acowley Oct 24, 2014
Owner

This seems like something that may be worth doing in bracket. The lifted-base package has suitable things, or we can do the plumbing ourselves since it's not too complicated.

-- ConHeadError is for an error with the connection header
data ServiceResponseExcept = NotOkExcept String | ResponseReadExcept String | MasterExcept String | ConHeadExcept String
deriving (Show, Eq)

This comment has been minimized.

@acowley

acowley Oct 24, 2014
Owner

This is great and we should update more of the code base to use custom error types like this.

generateMsgType :: ByteString -> [ByteString] -> Msg -> MsgInfo ByteString
generateMsgType pkgPath pkgMsgs msg =
generateMsgType = generateMsgTypeExtraImport ""

This comment has been minimized.

@acowley

acowley Oct 24, 2014
Owner

Maybe we should make a record type for the arguments to generateMsgTypeExtraImport as they're getting numerous, and denoting meaning with the function name doesn't scale too well. We could have a data type, PkgMsgs with fields extraImport, pkgPath, and pkgMsgs.

@acowley
Copy link
Owner

@acowley acowley commented Oct 24, 2014

This is an outstanding job! It's really great that not only will this add a really useful feature, but I think it's forcing improvements through the existing code you've had to touch.

where
handler x = return . Left . MasterExcept $
"Could not look up service with master. Is the ROS master (roscore) running? Got exception: " ++ show x

This comment has been minimized.

@acowley

acowley Oct 24, 2014
Owner

Could you replace return . left with throwError?

This comment has been minimized.

@rgleichman

rgleichman Oct 25, 2014
Author Collaborator

Doing that results in a type error. Since handler returns an IO action, throwError expects an IOException. This is because in the MonadError class the error has a functional dependency on the monad returned by throwError.

Couldn't match type `GHC.IO.Exception.IOException'
              with `ServiceResponseExcept'
When using functional dependencies to combine
  Control.Monad.Error.Class.MonadError
    GHC.IO.Exception.IOException IO,
    arising from the dependency `m -> e'
    in the instance declaration in `Control.Monad.Error.Class'
  Control.Monad.Error.Class.MonadError ServiceResponseExcept IO,
    arising from a use of `handler'
    at /home/robbie/git/robbie/roshask/src/Ros/Graph/Master.hs:55:55-61
In the second argument of `flip', namely `handler'
In the second argument of `(.)', namely
  `(flip catchIOError) handler'

This comment has been minimized.

@acowley

acowley Oct 25, 2014
Owner

Oh, good point! I had looked at the docs but read right over the fundep.

@rgleichman
Copy link
Collaborator Author

@rgleichman rgleichman commented Oct 25, 2014

The previous two commits should address the remaining comments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.