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 Solar units of measure #209

Merged
merged 2 commits into from
Mar 5, 2017
Merged

Added Solar units of measure #209

merged 2 commits into from
Mar 5, 2017

Conversation

robotsnowfall
Copy link
Contributor

@robotsnowfall robotsnowfall commented Mar 5, 2017

Measure Unit Reference
Length (Nominal) Solar Radius https://en.wikipedia.org/wiki/Solar_radius
Power Solar Luminosity https://en.wikipedia.org/wiki/Solar_luminosity
Mass Solar Mass https://en.wikipedia.org/wiki/Solar_mass

First time submission, let me know if I missed something.

@derekmorr derekmorr self-assigned this Mar 5, 2017
@derekmorr
Copy link
Collaborator

Thanks for the patch. The changes look good (I had to double check that some of the changes didn't belong in the radio package, but I think they're good where they are).

A few questions:

  • Should nominal solar radius be added as well?
  • Are there alternate symbols that don't involve unicode symbols? In the past, some users have raised objections about certain symbols being difficult to enter on a keyboard.

@robotsnowfall
Copy link
Contributor Author

I think having nominal solar radius would be useful, yes. I have a patch that adds it.

As for the unicode symbols, I totally see where you're coming from. I saw the symbol for Angstrom (Å) was already in the repo, so I didn't think it would be too much of a stretch. There doesn't seem to be generally accepted symbols other than the ones provided. I suppose we could make them up though? There would be a danger of overlap if we chose something like SR or RS for solar radius, or SL/LS for solar luminosity, etc.

@cquiroz
Copy link
Collaborator

cquiroz commented Mar 5, 2017

Thanks, it looks good to me.
I'm ok with unicode units if they are in common use or officially accepted for the unit

@derekmorr
Copy link
Collaborator

Ok, the patch looks good to me.

I'd rather not invent new symbols. If there are only unicode symbols, then we'll use them.

Thanks again for the PR!

@derekmorr derekmorr merged commit bf25830 into typelevel:master Mar 5, 2017
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.

3 participants