fix computerFileSize to parse correctly, prerapation to fix #25358 #27257
Conversation
|
@individual-it, thanks for your PR! By analyzing the history of the files in this pull request, we identified @PVince81, @felixheidecke and @butonic to be potential reviewers. |
individual-it
changed the title
| ['116.4 TB', 127983153473126], | ||
| ['116.4tb', 127983153473126] | ||
| ['116.4tb', 127983153473126], | ||
| ['8776656778888777655.4tb', 9.650036181387265e+30], |
There was a problem hiding this comment.
Looks like we're already ready for the next century's storage requirements 😆
There was a problem hiding this comment.
living in the foothills of the Himalayas I know about huge clouds, so better be prepared, who knows what the climate change will bring ;-)
core/js/js.js
Outdated
| var matches = s.match(/([kmgtp]?b?)$/i); | ||
| if (matches[1]) { | ||
| bytes = bytes * bytesArray[matches[1]]; | ||
| var matches = s.match(/^[\s+]?([0-9]*)(\.([0-9]+))?( +)?([kmgtp]?b?)$/i); |
There was a problem hiding this comment.
I suggest to pre-compile the regexp and store it in a hidden variable this._computerFileSizeRegexp or so.
I'm not sure if modern Javascript engines recompile the Regexp every time or whether it already caches it somewhere.
To be safe, store it somewhere so if this function is called in a loop it will run a bit faster.
There was a problem hiding this comment.
changed it. please check it
as suggested by @PVince81 in Review #27257 (comment)
|
Looks better 👍 |
|
thought that a user might enter values with white spaces, and that should be possible and valid |
|
Oh, screw Jenkins... I ran the JS tests locally and they pass. |
…27257) * fix computerFileSize to be mory picky * more tests for computerFileSize * codestyle fix * fix double initialization * pre-compile and store regexp in a hidden variable as suggested by @PVince81 in Review #27257 (comment) * test strings with whitespaces * trim string before parsing
|
This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Description
I've added some more tests for computerFileSize and they would fail, so fixed
the function to be more picky on what input to accept and try to parse and also fixed the rounding of values without units
Motivation and Context
@PVince81 mentioned this function in the context of fixing #25358 but it does not work correct
That are exact the problems we have in #25358
After this PR is accepted I'm happy to look into fixing #25358
How Has This Been Tested?
wrote more tests
Types of changes
Checklist: