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

Make Rock.Body types public #180

Closed
wants to merge 1 commit into from

Conversation

tmattio
Copy link
Collaborator

@tmattio tmattio commented Aug 30, 2020

Having the Body types private prevents users from implementing middlewares that make use of the body of the request. For instance, it is currently difficult to implement a custom logger.

@anuragsoni Is there any reason for making these types private? I don't want to undo a change that you made for a specific reason.

Related to #176

@anuragsoni
Copy link
Collaborator

Having the Body types private prevents users from implementing middlewares that make use of the body of the request. For instance, it is currently difficult to implement a custom logger.

I might be missing something here. What kind of middlewares would a a user not be able to implement with the current public mli? The user has access to getting content out of the body via to_string, to_stream etc. The example shared on #176 was about a poly variant that was part of Opium, not the Body type that was used internally. Maybe bringing back a compat module that retains the opium api from 0.18 release is worth it?

Is there any reason for making these types private? I don't want to undo a change that you made for a specific reason.

Looking at the mli again, i think instead of making the types private i should've made them abstract and added a val length : t -> int64. I am not sure we need to expose the internals of how the body type is represented for the module to be usable.
The intention with making the type private to allow for pattern matching on the body to extract the length and content, while ensuring things get created via the public api.

@tmattio
Copy link
Collaborator Author

tmattio commented Sep 9, 2020

#179 has been merged and made the Body types abstract. I'll open an issue to continue the discussion with a better description of the problem / use cases.

@tmattio tmattio closed this Sep 9, 2020
@tmattio tmattio deleted the make-body-type-public branch September 9, 2020 18:57
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