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.

Sign up for GitHub

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

Jump to bottom

Add support for long to ToQuantity #504

Closed
wants to merge 8 commits into from
Closed

Add support for long to ToQuantity #504

wants to merge 8 commits into from

Conversation

RobPethick
Copy link

@RobPethick RobPethick commented Dec 21, 2015 •
edited by clairernovotny
Loading

Fixes #503

This change is Reviewable

@mexx
Copy link
Collaborator

mexx commented Dec 21, 2015

Sorry, but what's the purpose to extend the domain range by allowing long but throw exceptions when this extended range is used? The right fix would be to provide ToWords for long, also see my comment

@RobPethick
Copy link
Author

@mexx sorry I hadn't seen your comment before putting up my PR. Obviously the point is to allow for it for ShowQuantityAs.None and ShowQuantityAs.Numeric. But I appreciate it kind of seems half complete.

@clairernovotny clairernovotny added this to the vNext milestone Jan 2, 2016
@RobPethick
Copy link
Author

added in ShowQuantityAs.Words

mexx
mexx reviewed Jan 17, 2016
View reviewed changes
src/Humanizer.v2.ncrunchsolution Outdated
@@ -0,0 +1,13 @@
<SolutionConfiguration>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this file needed? Isn't it only a configuration for NCrunch?

Copy link
Author

Choose a reason for hiding this comment

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

good catch, I have removed it

@mexx
Copy link
Collaborator

mexx commented Jan 19, 2016

I don't know should we mention this in the readme.md somehow?

@RobPethick
Copy link
Author

I could, though in general do we discuss types in the readme?

@mexx
Copy link
Collaborator

mexx commented Jan 19, 2016

You are right, there is no actual notion of the types in the readme.

LGTM, @onovotny can you have a look also and merge if it's all OK?

@clairernovotny clairernovotny self-assigned this Apr 18, 2017
@clairernovotny clairernovotny modified the milestones: v2.2, vNext Apr 18, 2017
@clairernovotny
Copy link
Member

Rebased and merged here: e88b07e, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@clairernovotny clairernovotny

Labels
enhancement
Projects
None yet
Milestone
v2.2
Development

Successfully merging this pull request may close these issues.
4 participants
@RobPethick @mexx @clairernovotny @hazzik