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

Avoid direct float comparisons with 0.0 in storage.rb #20871

Merged
merged 3 commits into from
Jan 6, 2021
Merged

Avoid direct float comparisons with 0.0 in storage.rb #20871

merged 3 commits into from
Jan 6, 2021

Conversation

djberg96
Copy link
Contributor

@djberg96 djberg96 commented Dec 7, 2020

Currently the rubocop linter gives us a handful of warnings for the Storage class like this:

app/models/storage.rb:621:5: W: Lint/FloatComparison: Avoid (in)equality comparisons of floats as they are unreliable. used_space.to_f == 0.0 ? 0.0 : (debris_size.to_f / used_space.to_f * 1000.0).round / 10.0

So, this PR just switches them to use the zero? method. Core Ruby appears to have its own code for checking floats vs. zero (which the zero? method uses):

https://github.com/ruby/ruby/blob/master/numeric.c#L1122-L1126

If we feel this isn't worthwhile, then I would vote to update our rubocop config to exclude these.

Update: originally I was only fixing the linter warnings, but I also went ahead and fixed the Style/FloatDivision warnings as well while I was here.

@@ -581,12 +581,12 @@ def used_space
alias_method :v_used_space, :used_space

def used_space_percent_of_total
total_space.to_f == 0.0 ? 0.0 : (used_space.to_f / total_space.to_f * 1000.0).round / 10.0
total_space.to_f.zero? ? 0.0 : (used_space.to_f / total_space * 1000.0).round / 10.0
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to cast total_space.to_f if we're just going to check for .zero? ? Wonder if total_space.zero? ? 0.0 : (used_space.to_f / total_space * 1000.0).round / 10.0 would be cleaner

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed, updated.

@agrare agrare self-assigned this Jan 6, 2021
@miq-bot
Copy link
Member

miq-bot commented Jan 6, 2021

Checked commits https://github.com/djberg96/manageiq/compare/a950a904541618be3be24d9cff83b18831fc99cd~...1b76d191843d09ae8df9715343150a1ccea6ca31 with ruby 2.6.3, rubocop 0.82.0, haml-lint 0.35.0, and yamllint
1 file checked, 0 offenses detected
Everything looks fine. 🍰

@agrare agrare merged commit 4d11d89 into ManageIQ:master Jan 6, 2021
@skateman
Copy link
Member

skateman commented Jan 7, 2021

Yeah, this broke CI on ui-classic:

  1) StorageController report_data for selected StorageCluster returns associated Storages
     Failure/Error: view.paged_view_search(session[:paged_view_search_options])
     
     NoMethodError:
       undefined method `zero?' for nil:NilClass
     # /home/skateman/Repositories/ManageIQ/manageiq/app/models/storage.rb:589:in `free_space_percent_of_total'

Apparently total_space can be nil

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants