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

Update deprecation message to suggest correct formatting. #1056

Conversation

nicholalexander
Copy link
Contributor

The current deprecation message for the before or after symbol suggests using a format string that includes spaces. This is a small change so that the deprecation warning is more useful.

[12] pry(main)> money.format(symbol_position: :before, symbol: true)
[ruby-deprecation] [DEPRECATION] `symbol_position: :before` is deprecated - you can replace it with `format: %u %n`

=> "$23.45"
[13] pry(main)> money.format(format: "%u %n", symbol: true)
=> "$ 23.45"
[14] pry(main)> money.format(format: "%u%n", symbol: true)
=> "$23.45"

Co-authored-by: Alec Clarke alec.clarke.dev@gmail.com

The current deprecation message for the before or after symbol suggests using
a format string that includes spaces.  This is a small change so that the
deprecation warning is more useful.

Co-authored-by: Alec Clarke <alec.clarke.dev@gmail.com>

context 'when the position is :before' do
it 'warns about deprecated :symbol_position' do
expect_any_instance_of(Money::FormattingRules).to receive(:warn)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note: We're aware that using any_instance_of may not be great - but given that the deprecation code is called on initialize and the spec helper warning modules are noops, it is difficult to assert against the deprecation warning strings.

We are up for alternatives on testing!

@semmons99 semmons99 merged commit ccddfdc into RubyMoney:main Oct 1, 2023
1 of 6 checks passed
@nicholalexander nicholalexander deleted the bug.fix-format-deprecation-warning-message branch October 1, 2023 19:54
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