Skip to content

Changes for 3.0.0 #9

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

Closed
FWeinb opened this issue Apr 12, 2016 · 21 comments
Closed

Changes for 3.0.0 #9

FWeinb opened this issue Apr 12, 2016 · 21 comments

Comments

@FWeinb
Copy link
Collaborator

FWeinb commented Apr 12, 2016

I just merged #8 which sets the basis for 3.0.0. Are there any other things we need to change for this release. We can change the API as we like because we are doing a major version bump, so getting the API straight is important.

@sandeepmistry
Copy link
Contributor

I think we should add an end() API which calls .stop() on the UDP socket.

We could also add a begin() and begin(port) API to override the port used. Maybe these can be optional?

@FWeinb
Copy link
Collaborator Author

FWeinb commented Apr 12, 2016

Adding end() sounds good. But I don't see the reason for begin, any idea why an NTP server should run on a port different from 123?

@sandeepmistry
Copy link
Contributor

Sorry, this was to set the local UDP port: _port.

@FWeinb
Copy link
Collaborator Author

FWeinb commented Apr 12, 2016

Sure that could be handy to change, good point.

@sandeepmistry
Copy link
Contributor

I'll get a PR done for this today.

@sandeepmistry
Copy link
Contributor

We also need to change the keywords.txt file, the IDE expects hard tabs instead of spaces.

@FWeinb
Copy link
Collaborator Author

FWeinb commented Apr 12, 2016

Didn't know that the IDE expects hard tabs there. Good to know. Did you change that in #10?

@sandeepmistry
Copy link
Contributor

Did you change that in #10?

No, I'll make the changes on master once #10 is merged.

@sandeepmistry
Copy link
Contributor

We should also add a getEpochTime() API.

@sandeepmistry
Copy link
Contributor

We should also add a getEpochTime() API.

I think this is might be what getRawTime() returns, but I need to double check tomorrow. I had to adjust the raw time by 70 years for this other library I was using with the NTPClient.

@sandeepmistry
Copy link
Contributor

I found out what was going on, I was creating an NTPClient as a local var instead of global. So, _timeOffset was not initialized to zero. I've correct this in 89aa335.

I think it would be worthwhile to also have a getEpochTime() API that is an alias to getRawTime(). PR coming shortly for this.

@sandeepmistry
Copy link
Contributor

sandeepmistry commented Apr 14, 2016

@FWeinb can we merge #11, #12, and #13 and tag a v3.0.0 release? Or are there other changes that are needed?

cc/ @obsoleted

@FWeinb
Copy link
Collaborator Author

FWeinb commented Apr 15, 2016

Looks all good! Thanks for your help!

@sandeepmistry
Copy link
Contributor

Cool! I've merged them in and also added a CHANGELOG.

I'll aim to tag a new 3.0.0 release early next week.

@FWeinb
Copy link
Collaborator Author

FWeinb commented Apr 15, 2016

Great! This is looking very good. I have to thank you again doing all the work. 👍

@FWeinb
Copy link
Collaborator Author

FWeinb commented Apr 16, 2016

I just looked at the current API and think we should change all these function to return integers instead of strings.

@sandeepmistry
Copy link
Contributor

I just looked at the current API and think we should change all these function to return integers instead of strings.

@FWeinb yes this makes sense, I've gone ahead and submitted #15.

@sandeepmistry
Copy link
Contributor

Is there anything else we need to get done before tagging a new release?

@FWeinb
Copy link
Collaborator Author

FWeinb commented Apr 18, 2016

That should it be. Thanks for the help!

@sandeepmistry
Copy link
Contributor

v3.0.0 has been released.

@FWeinb thanks for reviewing the API changes in this release!

@FWeinb
Copy link
Collaborator Author

FWeinb commented Apr 19, 2016

Great! Thank you for all your work.

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

No branches or pull requests

2 participants