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

Added Haddock documentation for rate units. #9

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mtesseract
Copy link

@mtesseract mtesseract commented Jun 15, 2017

Clearly state units for oneMinuteRate, fiveMinuteRate, fifteenMinuteRate and meanRate.
This should take care of #8

@mtesseract
Copy link
Author

Hi, any concerns about this PR?

@iand675
Copy link
Owner

iand675 commented Jun 23, 2017

Hi, sorry for the delay in responding. The meanRate value was in fact an bug arising from doing integer division rather than floating point division. It should be in seconds. I've also updated the moving average values to operate in seconds as well, as it the way it was reporting over 5 second intervals arose from a misunderstanding when I ported this library from the Java equivalent.

I will happily accept additional documentation, but it will need to be updated to reflect these changes.

@mtesseract
Copy link
Author

Hi Ian,

I've added two comments to your commit — looks like some testing code slipped in?

As I understand it, this commit breaks the semantics of the API. So it should include a major version bump.

Of course we can close this PR, as it's not applicable anymore.

Thanks,
Moritz

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