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

Expose NTP stratum #3

Merged
merged 1 commit into from
May 29, 2016
Merged

Expose NTP stratum #3

merged 1 commit into from
May 29, 2016

Conversation

knyar
Copy link
Contributor

@knyar knyar commented Apr 23, 2016

This exposes NTP stratum of the remote server and also allows returning other information as part of the Response struct.

@knyar
Copy link
Contributor Author

knyar commented May 11, 2016

@beevik, do you have any concerns about this?

@@ -26,6 +26,7 @@ const (
broadcast
controlMessage
reservedPrivate
maxStratum uint8 = 16
Copy link
Owner

Choose a reason for hiding this comment

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

Please move this to another const declaration scope. I prefer not to mix it in with the mode constants.

@knyar
Copy link
Contributor Author

knyar commented May 28, 2016

I've updated the commit to address your comments.

i := &Response{
m.Stratum,
m.ReceiveTime.UTC().Local(),
}
Copy link
Owner

Choose a reason for hiding this comment

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

sorry, one other minor change request. Could you rename this variable from "i" to "r" or "resp"? That'd be a bit more idiomatic.

Thanks.

@knyar
Copy link
Contributor Author

knyar commented May 28, 2016

Could you rename this variable from "i" to "r" or "resp"?

Sure, updated.

@beevik
Copy link
Owner

beevik commented May 29, 2016

Thanks for the pull request!

@beevik beevik merged commit 790674a into beevik:master May 29, 2016
@beevik beevik mentioned this pull request Jul 4, 2023
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