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

RSC7b add lib header #75

Merged
merged 1 commit into from Sep 4, 2018

Conversation

gernest
Copy link
Contributor

@gernest gernest commented Sep 4, 2018

closes #69

@ORBAT
Copy link
Contributor

ORBAT commented Sep 4, 2018

Do we want RestClient.NewHTTPRequest to be exported? Is it something that users of the package should be using?

@gernest
Copy link
Contributor Author

gernest commented Sep 4, 2018

@ORBAT We can't test it without exporting it. Also the users will be using it to create Request , so yep we should export it.

@ORBAT
Copy link
Contributor

ORBAT commented Sep 4, 2018

We can't test it without exporting it.

I'm not a believer in only unit-testing exported methods, or exporting names only to test them when they're in the same package as the tests.

Also the users will be using it to create Request

As far as I can tell, ably.Request creation doesn't need a *http.Request: func (c *RestClient) do(r *Request) calls c.NewHTTPRequest(r), so the only place where a "custom" HTTP request is needed is in an unexported method. In what other places are the requests from NewHTTPRequest needed by users of the library?

@gernest
Copy link
Contributor Author

gernest commented Sep 4, 2018

@ORBAT tests are in ably_test package so you can't test freely un exported symbols. This was one of the reason I had to do this.

And In my opinion the do of RestClient should be exported , however I didn't want to jam a lot of changes in one PR.

@rjeczalik
Copy link
Contributor

@gernest You can export newHTTPRequest just like post one is exported for test usage. For Request struct you can add a type alias there as well.

@ORBAT
Copy link
Contributor

ORBAT commented Sep 4, 2018

Personally I don't see the benefit of either NewHTTPRequest or do being exported, but I'm always willing to listen to differing opinions. What would the uses of an exported do be, @gernest?

@gernest
Copy link
Contributor Author

gernest commented Sep 4, 2018

@ORBAT the input params and returned values are both exported symbols for do . And just my personal preference I don't like black box libraries, meaning at least I should have the freedom to build my own abstraction over exposed functionality. Now do is the core and important part where actual calls are made.

Did you read where I am saying the test package is ably_test and that you cant test un exported symbols? Happy to pick another option if there is a better way to test newHTTPRequest

I really didn't have time to weigh on that as I am side tracked.

@ORBAT
Copy link
Contributor

ORBAT commented Sep 4, 2018

You have a good point about how lower-level APIs are nice to have. Let's keep this as it is, then

@ORBAT ORBAT self-requested a review September 4, 2018 12:22
@gernest
Copy link
Contributor Author

gernest commented Sep 4, 2018

@rjeczalik thanks. I thought about that too, I picked the easy route where instead of wrapping post into Post I can as well export Post and be done with it. That way I reduce duplicate functions, since the wrappers aren't doing anything else apart from wrapping .

Copy link
Contributor

@ORBAT ORBAT left a comment

Choose a reason for hiding this comment

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

Looks good. Thank you

@ORBAT ORBAT merged commit d03baa9 into ably:integration-spec-1.1 Sep 4, 2018
@gernest gernest deleted the rsc7b-add-lib-header branch September 4, 2018 12:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants