manageiq icon indicating copy to clipboard operation
manageiq copied to clipboard

[WIP] Use ActiveSupport::NumberHelper

Open kbrock opened this issue 3 years ago • 4 comments

This came out of https://github.com/ManageIQ/manageiq/pull/22426 /cc @mahantidinesh7

You used to need to use ActiveView::Helpers::NumberHelper to use these helper methods. You also used to need to create an instance object to call this helper. (We use added ApplicationController.helper for this purpose)

NumberHelper has since been moved from ActionView into ActiveSupport and it is available in class/module methods

changes:

  • number_with_delimiter has been renamed to number_to_delimited

We do monkey patch number_to_human via ui-classic NumberHelper. This adds support for negative numbers. From what I can tell, this patch is only available on ApplicationController#helper and we do not use that functionality here.

We also implemented format_mhz_to_human_size in NumberHelper. That has been mixed into ApplicationController#helper. That is our last reference to this vestige. Making that method available at a class level may be an option.


I did notice that report/formatting.rb has include ActionView::Helpers::NumberHelper in the global state. From what I can tell, we do not call this on any object, but that is why cross-repo tests are necessary. The CrossContamination test also suggests that this was not mixed into Object, I'm a little confused what the include actually does. (It definitely makes me nervious)

@miq-bot cross-repo-tests manageiq-ui-classic /stupid squirrels

kbrock avatar Mar 24 '23 17:03 kbrock

So the output is a little different. And it looks like for date helper, we'll need to use ApplicationController.helper (so it is not going away)

Options:

  1. punt
  2. drop commit 3
  3. move forward with option 3, and get the output close enough.

kbrock avatar Mar 24 '23 23:03 kbrock

WIP: these methods are slightly different. So it was not as quick a case. Also, since we can not drop ApplicationController.helpers, the value of this PR has been diminished.

It is a low priority and I will return later

kbrock avatar Apr 10 '23 19:04 kbrock

This pull request has been automatically marked as stale because it has not been updated for at least 3 months.

If these changes are still valid, please remove the stale label, make any changes requested by reviewers (if any), and ensure that this issue is being looked at by the assigned/reviewer(s)

Thank you for all your contributions! More information about the ManageIQ triage process can be found in the triage process documentation.

miq-bot avatar Jul 17 '23 00:07 miq-bot

Checked commits https://github.com/kbrock/manageiq/compare/8e31d0eadeb730943d5651e77fe27bf44bc8ff3b~...30fbdd621f3d3465a67e4def26f83e599983ca56 with ruby 2.7.8, rubocop 1.56.3, haml-lint 0.51.0, and yamllint 7 files checked, 0 offenses detected Everything looks fine. :star:

miq-bot avatar Mar 07 '24 20:03 miq-bot