这是indexloc提供的服务，不要输入任何密码
Skip to content

Handle negative numbers in NumberToHumanSizeConverter #49791

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Oct 26, 2023
Merged

Handle negative numbers in NumberToHumanSizeConverter #49791

merged 1 commit into from
Oct 26, 2023

Conversation

@Earlopain
Copy link
Contributor

@Earlopain Earlopain commented Oct 26, 2023

Motivation / Background

This Pull Request has been created because number_to_human_size doesn't handle negative numbers.

# Old behaviour
helper.number_to_human_size(-1234567)
# => "-1234567 Bytes"

# New behaviour
helper.number_to_human_size(-1234567)
# => "-1.18 MB"

Detail

number_to_human already supports negative numbers, this aligns the two methods.

Checklist

Before submitting the PR make sure the following are checked:

  • This Pull Request is related to one change. Changes that are unrelated should be opened in separate PRs.
  • Commit message has a detailed description of what changed and why. If this PR fixes a related issue include it in the commit message. Ex: [Fix #issue-number]
  • Tests are added or updated if you fix a bug or add a feature.
  • CHANGELOG files are updated for the changed libraries if there is a behavior change or additional feature. Minor bug fixes and documentation changes should not be included.

fatkodima
fatkodima reviewed Oct 26, 2023
View reviewed changes
activesupport/test/core_ext/numeric_ext_test.rb Outdated
Copy link
Member

@fatkodima fatkodima Oct 26, 2023

Choose a reason for hiding this comment

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

I see that some other test cases has lots of assertions, but I think that for this (and other added test case) just a few edge case assertions would be enough.

Copy link
Contributor Author

@Earlopain Earlopain Oct 26, 2023

Choose a reason for hiding this comment

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

I basically just copied them from the positive number test cases. I've cut down the number of cases being tested to a few less.

@Earlopain Earlopain force-pushed the number-human-size-negative branch from 6d92215 to 968c154 Compare October 26, 2023 08:31
jonathanhefner
jonathanhefner reviewed Oct 26, 2023
View reviewed changes
Copy link
Member

@jonathanhefner jonathanhefner left a comment

Choose a reason for hiding this comment

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

I'm inclined to fix this for the sake of mathematical purity, but, practically, what is the use case?

The only one I can think of is something rate related, e.g. "free space is decreasing at a rate of 1 GB per day" => "-1 GB / day". But I can't think of a time when I've actually seen something like that.

@Earlopain Earlopain force-pushed the number-human-size-negative branch from 968c154 to c6dcb11 Compare October 26, 2023 16:08
@Earlopain
Copy link
Contributor Author

Earlopain commented Oct 26, 2023 •
edited
Loading

I'm inclined to fix this for the sake of mathematical purity, but, practically, what is the use case?

The only one I can think of is something rate related, e.g. "free space is decreasing at a rate of 1 GB per day" => "-1 GB / day". But I can't think of a time when I've actually seen something like that.

In my application I have code like this in order to display the size difference between two files. One will either be larger or smaller in comparision to another.

number_to_human_size(entry.post_size - submission_file.size, precision: 2)

Currently I have the method monkey-patched to work around this issue.

@jonathanhefner jonathanhefner merged commit 87609e8 into rails:main Oct 26, 2023
@jonathanhefner
Copy link
Member

jonathanhefner commented Oct 26, 2023

In my application I have code like this in order to display the size difference between two files. One will either be larger or smaller in comparision to another.

Ah, yes, that's a good one! Thank you, @Earlopain! 💾

@Earlopain Earlopain deleted the number-human-size-negative branch October 26, 2023 16:23
rafaelfranca pushed a commit that referenced this pull request Oct 26, 2023
@jonathanhefner @rafaelfranca
Merge pull request #49791 from Earlopain/number-human-size-negative
6d55d50 
Handle negative numbers in `NumberToHumanSizeConverter`
jonathanhefner added a commit that referenced this pull request Oct 26, 2023
@jonathanhefner
Merge pull request #49791 from Earlopain/number-human-size-negative
a645b9f 
Handle negative numbers in `NumberToHumanSizeConverter`

(cherry picked from commit 87609e8)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jonathanhefner jonathanhefner jonathanhefner left review comments

@fatkodima fatkodima fatkodima left review comments

Assignees

No one assigned

Labels

actionview activesupport

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@Earlopain @jonathanhefner @fatkodima