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

Improve test coverage for FormattingHelper#format_money #2089

Merged
merged 1 commit into from
Nov 16, 2015

Conversation

floehopper
Copy link
Contributor

I believe that the origin of the code in FormattingHelper#format_money was
from the code which is now in SmartAnswer::I18nRenderer#value_for_interpolation.
Thus at least in some scenarios I would expect a SmartAnswer::Money object to be
passed in rather than a String. This commit adds assertions for the former.

Expected observable changes

  • None

@chrisroos
Copy link
Contributor

Looks good to me.

@chrisroos chrisroos added the LGTM label Nov 16, 2015
I believe that the origin of the code in `FormattingHelper#format_money` was
from the code which is now in
`SmartAnswer::I18nRenderer#value_for_interpolation`. Thus at least in some
scenarios I would expect a `SmartAnswer::Money` object to be passed in rather
than a `String`. This commit adds assertions for the former.

[1]: https://github.com/alphagov/smart-answers/blob/7e26c554be106b2888f44bf92746e5fc7f6d819f/lib/smart_answer/i18n_renderer.rb#L33-L34
@floehopper floehopper force-pushed the improve-test-coverage-for-formatting-helper-test branch from c908d30 to 8edbbe1 Compare November 16, 2015 11:23
@floehopper
Copy link
Contributor Author

I've rebased this against master and force-pushed in preparation for merging.

floehopper added a commit that referenced this pull request Nov 16, 2015
…atting-helper-test

Improve test coverage for FormattingHelper#format_money
@floehopper floehopper merged commit 98036c5 into master Nov 16, 2015
@floehopper floehopper deleted the improve-test-coverage-for-formatting-helper-test branch November 16, 2015 11:27
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.

2 participants