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

move exported types from C.D.P.P.Supervisor to .Types #80

Conversation

tavisrudd
Copy link
Contributor

This is in preparation for some changes to the handling of the
StarterProcess variants of ToChildStart. See discussion on PR #77.

This is in preparation for some changes to the handling of the
StarterProcess variants of ToChildStart. See discussion on PR haskell-distributed#77.
@hyperthunk
Copy link
Member

Thanks @tavisrudd - will try and get this merged asap. I'm not sure about exposing Supervisor.Types as a public top level module though. Is there really a need for third parties to know about all those Req and Respone types? As an alternative, if we do wish to just export everything and let people figure things out themselves, we could rename it to Supervisor.Internal.Types, but I feel that is of dubious merit if some of the types are meant to be public but some are not. We don't want people importing things from an Internal namespace that they need to use to get on with their day jobs! Of course, they can import the proper public types from Supervisor itself, but then it seems confusing to get both public and private types from something Internal. This was why on that other ticket, I suggested that the idea of just exporting everything inside an "Internal" namespace is not as simple as it necessarily sounds.

Splitting Supervisor.Types up into two bits, public vs private, seems unnecessarily complicated though - I'd recommend keeping the private things private. I don't see how any kind of extension points could be utilised by someone having access to supervisor's internal types here, so I think it's better to just keep private things hidden altogether in this case.

Thoughts?

@tavisrudd
Copy link
Contributor Author

I agree. I only added it to the exports as an afterthought when one of the tests failed because of it not being exported. I'll look into that failing test and remove it from the exposed modules.

Btw, this new module only exports the types that were previously public in Supervisor. I left the Request types private in Supervisor for now but did consider adding an additional Internal.Types. Only the public Result types are in here.

@tavisrudd
Copy link
Contributor Author

@hyperthunk it's on other-modules now, where I should have stuck it in the first place, but see my note above re it only containing public types.

@hyperthunk
Copy link
Member

Cool thanks @tavisrudd - I'll planning to go through outstanding PRs this weekend.

@hyperthunk hyperthunk merged commit c77372d into haskell-distributed:development Apr 20, 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.

2 participants