-
Notifications
You must be signed in to change notification settings - Fork 122
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
add formatter docs; minor adjustments for formatter syntax #244
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me. I added a couple of comments
@@ -709,6 +709,123 @@ The `UnitGroup` values provided with Squants are only samples and aren't intende | |||
We encourage users to make their own `UnitGroup` defintitions and submit them as PRs if they're generally | |||
applicable. | |||
|
|||
## Formatters | |||
|
|||
Squants provides an experimental API for formatting Quantities in the "best unit." For example, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder a bit about the experimental
part. If this is something we may change then the experimental
tag fits very well but then perhaps it should be on an experimental
package.
Do you foresee possible changes in the near future that could break the API?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure. We've not had any feedback on the API from people who requested it. Once it's released, it's possible we'll want to change it based on feedback.
I'm fine moving unitgroups and formatters into an experimental package.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess either way the API will change but the experimental would at least be an indication.
I don't have a strong opinion either way
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll refactor it into an experimental package and update the README.
shared/src/main/tut/README.md
Outdated
|
||
Then create the formatter by passing in a unit group: | ||
```tut:book | ||
val astroFormatter = new DefaultFormatter[Length] { val unitGroup = AstronomicalLengthUnitGroup } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why this isn't built just as a regular class? new DefaultFormatter[Length](AstronomicalLengthUnitGroup)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably b/c everything else was a trait. I agree the class syntax is better.
I pushed up a commit that addresses the latest comments. |
that seems good, we can move it to non |
Are we ok to merge this then? |
I think so |
This adds docs for formatters. I made some changes since the last PR to make the syntax more consistent.
I'd appreciate feedback on the docs. It can get complicated with unitgroups, formatters, implicits, etc. I'm not sure I structured it in the best way.