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

buffer: expose internals on binding #770

Closed
wants to merge 1 commit into from

Conversation

vkurchatkin
Copy link
Contributor

Remove internal object and expose functions directly on binding.
This makes possible to simply use internal functions in other builtin
modules.

R=@trevnorris

Remove `internal` object and expose functions directly on binding.
This makes possible to simply use internal functions in other builtin
modules.
@trevnorris
Copy link
Contributor

The point of the approach was to make sure code that could abort() from an argument check wasn't exposed to the user, but since it's already behind process.binding() and anything there is considered internal anyway I don't see the harm here. LGTM.

@rvagg
Copy link
Member

rvagg commented Feb 10, 2015

@trevnorris don't we have existing bindings that are strict with their arguments that are exposed in this way? If we do then this LGTM but if we don't already have an example to follow then I'm not enthusiastic.

@vkurchatkin
Copy link
Contributor Author

@rvagg not sure what you are talking about. This is consistent with how other bindings are implemented

@vkurchatkin
Copy link
Contributor Author

@rvagg ping. do you still have objections to this?

@bnoordhuis
Copy link
Member

LGTM

vkurchatkin added a commit that referenced this pull request Feb 11, 2015
Remove internal object and expose functions directly on binding.  This
makes possible to simply use internal functions in other builtin
modules.

PR-URL: #770
Reviewed-by: Trevor Norris <[email protected]>
Reviewed-by: Ben Noordhuis <[email protected]>
@trevnorris
Copy link
Contributor

Landed in 36a7795.

@trevnorris trevnorris closed this Feb 11, 2015
@rvagg rvagg mentioned this pull request Feb 18, 2015
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.

4 participants