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 byte rate calculation functons, closes #296 #314

Closed
wants to merge 4 commits into from

Conversation

GeorgeHahn
Copy link
Contributor

API is var rateString = bytesize.Per(timespan).ToRatePerSecond();

{
public class ByteRate
{
public ByteSize size { get; private set;}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please change the property names to CamelCase.

@mexx
Copy link
Collaborator

mexx commented Jul 6, 2014

The CI build is failing because of missing XML comments on ByteSize class and its properties.
Please add them.

Also please review contribution guidelines

  • Link to the issue(s) you're fixing from your PR description. Use fixes #<the issue number>
  • Readme is updated if you change an existing feature or add a new one
  • An entry is added in the release_notes.md file in the 'In Development' section with a link to your PR and a description of what's changed. Please follow the wording style for the description.

var rate = size.Per(measurementInterval);
var text = rate.Humanize(displayInterval);

text = size.Per(measurementInterval).Humanize(displayInterval);
Copy link
Member

Choose a reason for hiding this comment

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

I am not sure why you're setting text, size and measurementInterval variables here again!! I am removing them. Please remove them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Shoot, I was playing with the API and totally forgot to remove it when I was done!

@MehdiK
Copy link
Member

MehdiK commented Jul 7, 2014

You have a failing test. Please run all the tests and fix the ApiApprover test.

@GeorgeHahn
Copy link
Contributor Author

The ApiApprover test doesn't run on my machine, but I've taken a crack at fixing it. I believe I've addressed the other issues noted above.

E: Looks like I failed to fix the ApiApprover test. It doesn't look like I can download the generated text file that's being tested against from TC, is there any chance you could gist it?

@@ -8,6 +8,7 @@
- [#307](https://github.com/MehdiK/Humanizer/pull/307): Added support to string.FormatWith for the explicit culture parameter
- [#312](https://github.com/MehdiK/Humanizer/pull/312): Added Turkish ToWord, ToOrdinalWord and Ordinalize implementation
- [#173](https://github.com/MehdiK/Humanizer/pull/173): Added support for Window Phone 8.1
- [#314](https://github.com/MehdiK/Humanizer/pull/314): Added ByteRate class and supporting members to facilitate calculation of byte transfer rates
Copy link
Collaborator

Choose a reason for hiding this comment

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

This line should move to the development section.
@MehdiK why do you release stuff while other are still developing 😠 :ironic:

@MehdiK
Copy link
Member

MehdiK commented Jul 7, 2014

Looks like I failed to fix the ApiApprover test. It doesn't look like I can download the generated text file that's being tested against from TC, is there any chance you could gist it?

When the test fails you should see a diff tool popping up showing the expected text. Not sure why it's not working on your machine! Can you run it using a different runner?

Please rebase your work on top of the upstream & move the release notes up to the dev section. Thanks.

@mexx mexx closed this in b6b03bc Aug 7, 2014
@mexx
Copy link
Collaborator

mexx commented Aug 7, 2014

Finally I've found some time to fix the ApiApprover test.
@GeorgeHahn Thanks for your contribution.

@GeorgeHahn
Copy link
Contributor Author

Thanks Max! My apologies for not fixing the test myself. I've tried it on three different pc's since the dialog here and have yet to see it run correctly.

On Thu, Aug 7, 2014 at 5:12 PM, Max Malook notifications@github.com
wrote:

Finally I've found some time to fix the ApiApprover test.

@GeorgeHahn Thanks for your contribution.

Reply to this email directly or view it on GitHub:
#314 (comment)

@MehdiK
Copy link
Member

MehdiK commented Aug 16, 2014

@mexx thanks for fixing, rebase and pushing this up. You're awesome. Just one small thing: please use merge --no-ff to retain the PR bump on the commit graph.

@MehdiK
Copy link
Member

MehdiK commented Sep 12, 2014

This is now released to NuGet as v1.29.0. Thanks for the great addition and your collaboration.

Borzoo pushed a commit to Borzoo/Humanizer that referenced this pull request Dec 23, 2014
Borzoo pushed a commit to Borzoo/Humanizer that referenced this pull request Dec 23, 2014
Borzoo pushed a commit to Borzoo/Humanizer that referenced this pull request Dec 23, 2014
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.

None yet

3 participants