search

radar / humanize

Takes your numbers and makes them *fancy*.
MIT License
468 stars 85 forks source link

Separate core_ext #88

Closed ColinDKelley closed 6 months ago

ColinDKelley commented 6 months ago

NOTE Best reviewed by skipping the first commit that moved humanize.rb to humanize/module.rb and ignoring whitespace: https://github.com/radar/humanize/pull/88/files/a84b371db91fe64c184215df1b147f7a70c8912d..500e185faeb990f4e801d07ec6ac20643249f540

Changes

The Humanize gem would formerly always duck punch itself into the Integer, Float, and BigDecimal classes.

This PR takes inspiration from how active_support does core extensions. The core extensions are now optional with this require:

require 'humanize/core_ext'

And that is required automatically if you

require 'humanize'

to match previous behavior. But new code has the option to set require: false in the Gemfile and just do

require 'humanize/module'

Then rather than doing

42.humanize(...)

it can instead use the pure functional form (which is always available):

Humanize.format(42, ...)

This is useful for code bases that want don't want to duck punch into Ruby core classes.

radar commented 6 months ago

Hey, thanks for this! I think it's a great PR.

Would you mind looking into the build failures here?

ColinDKelley commented 6 months ago

@radar BTW I think the # frozen_string_literal: true magic comment can help across the board here. I won't clutter this PR with that, but once this is merged, I'll submit another PR that includes that magic comment and then drops .freeze after all the string constants.

radar commented 6 months ago

Fine by me regarding FSL.

Rubocop is being a grump. Would you mind fixing that up? I can't push to your branch due to permission restrictions.

ColinDKelley commented 6 months ago

Rubocop is being a grump. Would you mind fixing that up? I can't push to your branch due to permission restrictions.

Ah, I'd made this change suppress that rubucop failure but lost it in the merge-up. Merged back here: https://github.com/radar/humanize/pull/88/commits/a347bbb140a6a2d1f1af643c13e6a771e1b57b60

Also, I found I'd missed this recursive call that was depending on the core Integer class being extended. I've switched it to functional (which will always work): https://github.com/radar/humanize/pull/88/commits/9339246a259e90b046248080726e43e89e496137

And finally, I updated the README to document the new functional usage: https://github.com/radar/humanize/pull/88/commits/32bd066d754d4e04b4ad431ff9268ee4d49d7411

radar commented 6 months ago

All checks out to me. Thanks very much! ❤️