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

Byte size #99

Merged
merged 11 commits into from
Mar 17, 2014
Merged

Byte size #99

merged 11 commits into from
Mar 17, 2014

Conversation

GeorgeHahn
Copy link
Contributor

PR as requested, will be able to jump back in tomorrow!

  • Rest of tests
  • Refactor namespaces all around
  • Add API to readme
  • Licensing

@GeorgeHahn
Copy link
Contributor Author

Oy. Judging by the extra changes shown, I think I may have incorrectly pulled in changes from PR #103. Should I close this one and PR against the bytesize branch?

@GeorgeHahn
Copy link
Contributor Author

Do we need any other tests here? The ByteSize library is fairly well tested by itself, and the glue between the two libraries is minimal.

I'd rather not have to do a whole bunch of copypasta extensions, but it seems it would be valuable to add overloads for int on all extensions, else the compiler won't allow 2.xxBytes()

I dropped Dehumanize Support from the todo list, I suspect it would be rare to need such functionality. What's your opinion?

@MehdiK
Copy link
Member

MehdiK commented Mar 16, 2014

A few things went on while this PR was waiting; but you seem to have pulled them all in. So that's cool. Thanks.

I haven't gone through tests TBH. We do need a few tests for Humanize methods to make sure the two connect nicely. Also the existing tests from ByteSize look very different to Humanizer ones; so they should be refactored for consistency.

I think we could skip over Dehumanize for now. If someone thinks that's needed they can create an issue (and send a PR ;)).

Thanks for the great work. I will check it out later. Very busy atm.

@MehdiK MehdiK mentioned this pull request Mar 16, 2014
@MehdiK MehdiK merged commit 5485f4c into Humanizr:master Mar 17, 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.

2 participants