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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add delimiter option for number fields #1797

Merged
merged 1 commit into from Oct 29, 2020

Conversation

ianbayne
Copy link
Contributor

@ianbayne ianbayne commented Oct 14, 2020

Great gem, everyone! We're using it in a project currently and are finding it very helpful.

I realized there doesn't seem to be a delimiter option for number fields to, for example, display a number like 1000000 in the more legible format of 1,000,000, so I whipped up this PR to add it in. Please let me know if there's anything else I can or should add. 馃檱

data.nil? ? "-" : format_string % value
result = data.nil? ? "-" : format_string % value
result = add_delimiter(result) if options[:delimiter]
prefix + result + suffix
Copy link
Contributor Author

@ianbayne ianbayne Oct 14, 2020

Choose a reason for hiding this comment

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

I realized that the delimiter option works fine as originally coded, but only when there isn't a prefix or suffix. Commit b163adb ensures it works with both and adds a test to ensure that that is the case.


def add_delimiter(result)
ActiveSupport::NumberHelper.
number_to_delimited(result, delimiter: options[:delimiter])
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure about the user passing a specific delimiter explicitly. In theory, this should be provided by the locale. In fact number_to_delimited should by the default format as per the locale, so it shouldn't need a delimiter either.

So in my mind, :delimiter should be just a boolean option. Thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

It would be great to have a way to give various options that then get forwarded to number_to_delimited:

  • delimiter
  • separator
  • delimiter_pattern

However the same could be said for other ActiveSupport methods, e.g number_to_percentage:

  • precision
  • significant
  • separator
  • delimiter
  • strip_insignificant_zeros
  • format
  • number_to_rounded, number_to_phone, number_to_currency, number_to_human_size, number_to_human. So even if all of that is not enabled in this PR it might be good to have an API that does not prevent these other possibilities.

For example this field could accept two options:

  • conversion_type with possible values number_to_delimited, number_to_percentage, etc
  • conversion_options would be passed transparently to the relevant ActiveSupport method

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the comments, @pablobm & @sedubois.

In theory, this should be provided by the locale. In fact number_to_delimited should by the default format as per the locale, so it shouldn't need a delimiter either. So in my mind, :delimiter should be just a boolean option. Thoughts?

Personally, while I imagine that in the majority of cases users will default to whatever is standard for their locale, when using Administrate I would appreciate the added flexibility of being able to choose my own delimiter.

It would be great to have a way to give various options that then get forwarded to number_to_delimited

I like this idea, and the idea of leaving the API "ready" for future updates to allow other ActiveSupport methods. On note, however, is that as there's already a decimals option, if we were to do so, we'd need to make sure that separator from number_to_delimited wouldn't step on its toes.

It looks like there are three options.

  1. Do as @pablobm suggests and convert :delimiter to just being a boolean.
  2. Keep as is (i.e., explicit defining of delimiter), perhaps adding the option to pass true to default to the delimiter for the user's locale.
  3. Update to allow passing of various options to number_to_delimited, as @sedubois suggests.

Thoughts? I'm happy to implement any of these the maintainers/etc. agree is the preferred direction they/you want to take Administrate.

Copy link
Collaborator

@pablobm pablobm Oct 22, 2020

Choose a reason for hiding this comment

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

Good point on other available number helpers, @sedubois! This does complicate things. Still, I think it's worth looking into this issue, as Number is already a bit locale-unfriendly with its use of a format string like "%.#{decimals}f".

The idea of having two arguments conversion_type and conversion_options sounds appealing to me. One step further would be to allow a lambda to do the conversion, but I feel that could go too far, and Number could become a "universal" type of field.

Perhaps use different names, like formatter and formatter_options? I think this describes their intent better.

Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps use different names, like formatter and formatter_options

LGTM 馃憤

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, @pablobm. I've now implemented this. Please take a look when you get the chance.

@ianbayne ianbayne force-pushed the new/add-delimiter-option branch 2 times, most recently from b3f8a93 to 0abef82 Compare October 23, 2020 02:46
@ianbayne ianbayne requested a review from pablobm October 23, 2020 02:51
Copy link
Collaborator

@pablobm pablobm 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! Thank you,

This makes it so that we can display a number like `1000000` in the more
legible format of `1,000,000`.
@nickcharlton nickcharlton merged commit edfc630 into thoughtbot:master Oct 29, 2020
@ianbayne ianbayne deleted the new/add-delimiter-option branch October 30, 2020 00:16
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.

None yet

4 participants