Skip to content

Conversation

@ALPSMAC
Copy link

@ALPSMAC ALPSMAC commented Feb 15, 2018

First real attempt at a PR here for Squants (or much of anything for that matter). Trying to be helpful, so please be gentle ;-)

See issue #298

Thanks and Kind Regards,
Andy

Copy link
Collaborator

@cquiroz cquiroz left a comment

Choose a reason for hiding this comment

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

Sorry for the late review. This seems good to me. I'll wait a few days for comments from other committers before merging

@derekmorr
Copy link
Collaborator

I'm reluctant to accept this. For every other unit of measure in the library, there's a standard that defines it (well, aside from the bitcoin currency). Kiloyards isn't a standard, though.

I understand that the current library design makes adding custom units more cumbersome than it needs to be.

@cquiroz
Copy link
Collaborator

cquiroz commented Apr 26, 2018

I guess that is a valid point. Being non-US it's difficult for me to tell if Kiloyards is a thing

@ALPSMAC
Copy link
Author

ALPSMAC commented Apr 27, 2018

@cquiroz for my employer it is a thing, and used more broadly in a nautical context in the U.S., although I think its more a historical norm than a real international standard.

On reflection I believe it would be sufficient to represent it as 1000 yards (for a kiloyard) or 1000 feet (for a kilofoot) and we can probably get by with that.

@derekmorr out of curiosity, what is the preferred way to go about adding additional units to this library without having to change the library itself? I see this and this from the README.md but based on what I could grok from the description I'm not sure that's really what I'm looking for.

P.S. Thank you for Squants - this library has been a lifesaver! I can't begin to quantify the number of bugs this has squashed before they ever had a chance to manifest.

@garyKeorkunian
Copy link
Contributor

@ALPSMAC You can create additional units outside of Squants by extending the LengthUnit much as you did in the PR. You don't everything that way (eg, string parsing, DSL support), but you can the unit is useful.

As for this PR, do you have any reference to Kiloyards or Kilofeet?

@garyKeorkunian
Copy link
Contributor

@ALPSMAC - Outside of the fact that these units may not be "standard enough" for this library, the PR is very well done. Thank you for your contribution.

@derekmorr @cquiroz Any additional thoughts on this?

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.

4 participants