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

API CHANGE: NewClient(clientName, clientVersion) #21

Merged
merged 5 commits into from Jul 24, 2019
Merged

Conversation

bassosimone
Copy link
Contributor

This commit changes the NewClient API to take two arguments, the
clientName and the clientVersion, instead of a userAgent.

We didn't specify how userAgent was made. In theory we expected it
to be composed of ${clientName}/${clientVersion} but that was not
enforced in any way. Per #16, we want the client to identify with
the server also using the query string.

This is for consistency with what the JavaScript client does.

While there, also provide information on the OS and the architecture
of the client, again as part of the query string.

Closes #16.

This commit changes the NewClient API to take two arguments, the
clientName and the clientVersion, instead of a userAgent.

We didn't specify how userAgent was made. In theory we expected it
to be composed of `${clientName}/${clientVersion}` but that was not
enforced in any way. Per #16, we want the client to identify with
the server also using the query string.

This is for consistency with what the JavaScript client does.

While there, also provide information on the OS and the architecture
of the client, again as part of the query string.

Closes #16.
@bassosimone bassosimone requested a review from pboothe July 6, 2019 09:05
@bassosimone bassosimone self-assigned this Jul 6, 2019
@bassosimone bassosimone added the enhancement New feature or request label Jul 6, 2019
@bassosimone bassosimone marked this pull request as ready for review July 6, 2019 09:05
@pboothe
Copy link
Contributor

pboothe commented Jul 23, 2019

Please resolve the conflicts?

@bassosimone
Copy link
Contributor Author

Sure!

@bassosimone
Copy link
Contributor Author

@pboothe done!

Copy link
Contributor

@pboothe pboothe left a comment

Choose a reason for hiding this comment

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

LGTM, with some wondering about what your formatter is doing.

ndt7.go Outdated Show resolved Hide resolved
@bassosimone bassosimone merged commit ad7eefc into master Jul 24, 2019
@bassosimone bassosimone deleted the issue/16 branch July 24, 2019 15:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ndt7-client should identify itself with the server
2 participants