Skip to content

Conversation

@RubenIgnacio
Copy link
Contributor

Refactor the Monetize::Parser class to store the parsed currency in an instance variable during initialization. This simplifies the method calls by removing the need to pass the currency as an explicit argument.

yukideluxe
yukideluxe previously approved these changes Jan 22, 2026
Copy link
Member

@yukideluxe yukideluxe left a comment

Choose a reason for hiding this comment

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

looks good to me! Thanks, @RubenIgnacio!

what do you think, @sunny?

Copy link
Member

@sunny sunny left a comment

Choose a reason for hiding this comment

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

I like these small changes that add readibility and are easy to review \o/

@input = input.to_s.strip
@fallback_currency = fallback_currency
@options = options
@currency = Money::Currency.wrap(parse_currency)
Copy link
Member

Choose a reason for hiding this comment

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

What do you think about making that into a private method with memoisation instead of an attr_reader? E.g.:

def currency
  @currency ||= Money::Currency.wrap(parse_currency)
end

It doesn’t make a big difference but in the rare case where we don’t use that method, the parsing won’t be called.

Copy link
Member

Choose a reason for hiding this comment

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

I really have no preference, I also like that approach and we just leave initialize for instantiating the arguments 🙆🏻‍♀️

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That makes total sense for me, I’ll update the code.

I have a question, I actually have a few similar changes that are chained to this branch. Should I open them as separate PRs even if they depend on this one?

Copy link
Member

Choose a reason for hiding this comment

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

What would be lovely would be chaining them by making PRs where the base is the previous PR, this way the diff is only focused on one change.

Copy link
Member

Choose a reason for hiding this comment

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

thanks!

Copy link
Member

@sunny sunny left a comment

Choose a reason for hiding this comment

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

👏🏻

@sunny sunny merged commit 260851b into RubyMoney:main Jan 22, 2026
6 checks passed
@RubenIgnacio RubenIgnacio deleted the instance_variable_currency branch January 22, 2026 15:00
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