-
Notifications
You must be signed in to change notification settings - Fork 985
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
Fixes #16577 - enhance unit detection #3854
Conversation
test '#to_gb non matching string raises exception with correct message' do | ||
value = 'something that is not matched' | ||
exception = assert_raises(RuntimeError) { value.to_gb } | ||
assert exception.message =~ /^Unknown string/, "wrong exception reason #{exception.to_s}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Redundant use of Object#to_s in interpolation.
@@ -140,17 +140,20 @@ def to_translation | |||
end | |||
|
|||
def to_gb | |||
value, _, unit=self.match(/(\d+(\.\d+)?) ?(([KMGT]i?B?|B))$/i)[1..3] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this used to fail, #match return nil if there's no match and we called [1..3]
on it
@@ -140,17 +140,20 @@ def to_translation | |||
end | |||
|
|||
def to_gb | |||
value, _, unit=self.match(/(\d+(\.\d+)?) ?(([KMGT]i?B?|B))$/i)[1..3] | |||
match_data = self.match(/(\d+(\.\d+)?) ?(([KMGT]i?B?|B|Bytes))$/i) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't the match start at the start of the string?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't want to mix refactorings with fix since I thought this should be merged into 1.13 but since it was rejected I can fix existing code.
value, _, unit = match_data[1..3] | ||
else | ||
raise "Unknown string: #{self.inspect}!" | ||
end | ||
case unit.to_sym |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we use case insensitive matching, so for example "10gb" will be caught, but there is no :gb
symbol - perhaps .lower.to_sym
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
alright, even though gB is not gb
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
technically GiB isn't the same as GB either, but we ignore that as well :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
right :-)
thanks @tbrisker, I made changes you requested, could you take another look? |
[test foreman] |
[test foreman] |
No description provided.