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

add precision option #134

Merged
merged 2 commits into from
Dec 13, 2016
Merged

add precision option #134

merged 2 commits into from
Dec 13, 2016

Conversation

Neodelf
Copy link
Contributor

@Neodelf Neodelf commented Dec 9, 2016

Add precision option to #to_words

@coveralls
Copy link

Coverage Status

Coverage increased (+0.06%) to 99.88% when pulling 5ab7f1e on Neodelf:precision_option into 0c3a3af on kslazarev:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage increased (+0.06%) to 99.88% when pulling 5ab7f1e on Neodelf:precision_option into 0c3a3af on kslazarev:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.06%) to 99.88% when pulling 5ab7f1e on Neodelf:precision_option into 0c3a3af on kslazarev:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage increased (+0.06%) to 99.88% when pulling 5ab7f1e on Neodelf:precision_option into 0c3a3af on kslazarev:master.

@dblock
Copy link
Collaborator

dblock commented Dec 9, 2016

How do I exercise this from code?

@Neodelf
Copy link
Contributor Author

Neodelf commented Dec 9, 2016

@dblock, you might execute 7.1.to_words(precision: 3)

@dblock
Copy link
Collaborator

dblock commented Dec 10, 2016

I would add a test that calls that @Neodelf. Next this needs a README and a CHANGELOG update, please. And the build to pass.

@dblock
Copy link
Collaborator

dblock commented Dec 10, 2016

I would just remove rbx from travis.
If a newer version of JRuby doesn't just work, remove that too.

@Neodelf
Copy link
Contributor Author

Neodelf commented Dec 10, 2016

@dblock you want to remove 'rbx' and 'jruby' versions in '.travis.yml`?
Tests covers new added code (but only russian version or is not true?). Yes, I will change README and CHANGELOG in very soon.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.06%) to 99.88% when pulling 6dd84ee on Neodelf:precision_option into 0c3a3af on kslazarev:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage increased (+0.06%) to 99.88% when pulling 6dd84ee on Neodelf:precision_option into 0c3a3af on kslazarev:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.06%) to 99.88% when pulling 3e28967 on Neodelf:precision_option into 0c3a3af on kslazarev:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage increased (+0.06%) to 99.88% when pulling 3e28967 on Neodelf:precision_option into 0c3a3af on kslazarev:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.06%) to 99.88% when pulling 421380f on Neodelf:precision_option into 0c3a3af on kslazarev:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage increased (+0.06%) to 99.88% when pulling 421380f on Neodelf:precision_option into 0c3a3af on kslazarev:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.06%) to 99.88% when pulling 421380f on Neodelf:precision_option into 0c3a3af on kslazarev:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage increased (+0.06%) to 99.88% when pulling 421380f on Neodelf:precision_option into 0c3a3af on kslazarev:master.


* Your contribution here.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Put this line back ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

@dblock
Copy link
Collaborator

dblock commented Dec 10, 2016

I just want the build to pass, and I don't care enough about JRuby and definitely not about RBX to waste your time. From tests it's not clear that you can do to_words(precision: ...), so I was hoping for one just like that in any language.

@Neodelf
Copy link
Contributor Author

Neodelf commented Dec 10, 2016

From tests it's not clear that you can do to_words(precision: ...)

I just followed convention about existed tests to order to new tests are easy to fit in existence

so I was hoping for one just like that in any language.

What do you suggest?

@coveralls
Copy link

Coverage Status

Coverage increased (+0.06%) to 99.88% when pulling c35047f on Neodelf:precision_option into 0c3a3af on kslazarev:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage increased (+0.06%) to 99.88% when pulling c35047f on Neodelf:precision_option into 0c3a3af on kslazarev:master.

@dblock
Copy link
Collaborator

dblock commented Dec 11, 2016

Looking at existing tests maybe I'm asking for something we don't need. I'm happy with the PR as long as you can please help fix the build?

@coveralls
Copy link

Coverage Status

Coverage increased (+0.06%) to 99.88% when pulling 41bee13 on Neodelf:precision_option into 0c3a3af on kslazarev:master.

@coveralls
Copy link

coveralls commented Dec 11, 2016

Coverage Status

Coverage decreased (-0.3%) to 99.52% when pulling 41bee13 on Neodelf:precision_option into 0c3a3af on kslazarev:master.

@coveralls
Copy link

coveralls commented Dec 11, 2016

Coverage Status

Coverage decreased (-0.3%) to 99.52% when pulling bd95f1a on Neodelf:precision_option into 0c3a3af on kslazarev:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.06%) to 99.88% when pulling a8fed9f on Neodelf:precision_option into 0c3a3af on kslazarev:master.

@coveralls
Copy link

coveralls commented Dec 11, 2016

Coverage Status

Coverage decreased (-0.3%) to 99.52% when pulling a8fed9f on Neodelf:precision_option into 0c3a3af on kslazarev:master.

@dblock
Copy link
Collaborator

dblock commented Dec 12, 2016

Just remove rbx or add it to allow_failures, it's not worth it.

@coveralls
Copy link

coveralls commented Dec 12, 2016

Coverage Status

Coverage decreased (-0.3%) to 99.52% when pulling 4ef2f89 on Neodelf:precision_option into 0c3a3af on kslazarev:master.

@Neodelf
Copy link
Contributor Author

Neodelf commented Dec 12, 2016

@dblock yes, I removed it)

@@ -15,3 +15,16 @@ to_words:
21.77: двадцать одна целая и семьдесят семь сотых
111.999: сто одиннадцать целых и девятьсот девяносто девять тысячных
4242.7463: четыре тысячи двести сорок двe целых и семь тысяч четыреста шестьдесят три десяти тысячных
fractions_with_precision:
options:
:precision: 2
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't this be precision: 2? (as well as the others)

Copy link
Contributor Author

@Neodelf Neodelf Dec 12, 2016

Choose a reason for hiding this comment

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

Unfortunately, no

@coveralls
Copy link

Coverage Status

Coverage increased (+0.06%) to 99.88% when pulling 87a8bc3 on Neodelf:precision_option into 0c3a3af on kslazarev:master.

@coveralls
Copy link

coveralls commented Dec 12, 2016

Coverage Status

Coverage decreased (-0.3%) to 99.52% when pulling 87a8bc3 on Neodelf:precision_option into 0c3a3af on kslazarev:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.06%) to 99.88% when pulling 7f5cabd on Neodelf:precision_option into 0c3a3af on kslazarev:master.

@coveralls
Copy link

coveralls commented Dec 12, 2016

Coverage Status

Coverage decreased (-0.3%) to 99.52% when pulling 7f5cabd on Neodelf:precision_option into 0c3a3af on kslazarev:master.

@jlduran
Copy link
Collaborator

jlduran commented Dec 12, 2016

I made a couple minor suggestions, but it's OK as it is.

@dblock dblock merged commit c24a2ed into kslazarev:master Dec 13, 2016
@dblock
Copy link
Collaborator

dblock commented Dec 13, 2016

Merged.

jlduran added a commit that referenced this pull request Jun 11, 2017
Features

- Add full support for Brazilian Portuguese.  [#139]
- Add option `precision` to `to_words`.  [#134]

Bugs

- Fix README typo for 231 in French.
- Fix big numbers in Spanish.  [#129]
- Fix typo for number twenty-one in Spanish.  [#127]
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