-
Notifications
You must be signed in to change notification settings - Fork 130
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
Update function usage in SSR and remove need for commons-io #13073
Conversation
…o readable file size.
Project dependencies changesThe following changes in project dependencies were detected (configuration list
tree-\--- commons-io:commons-io:2.11.0 |
📲 You can test the changes from this Pull Request in WooCommerce-Wear Android by scanning the QR code below to install the corresponding build.
|
📲 You can test the changes from this Pull Request in WooCommerce Android by scanning the QR code below to install the corresponding build.
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## trunk #13073 +/- ##
=========================================
Coverage 40.41% 40.41%
Complexity 6194 6194
=========================================
Files 1294 1294
Lines 74479 74485 +6
Branches 10188 10189 +1
=========================================
+ Hits 30097 30103 +6
Misses 41771 41771
Partials 2611 2611 ☔ View full report in Codecov by Sentry. |
WooCommerce/src/main/kotlin/com/woocommerce/android/extensions/WCSSRModelExt.kt
Show resolved
Hide resolved
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'm thinking if we should use com.zendesk.util.FileUtils.humanReadableFileSize.humanReadableFileSize()
since we're already using this Zendes library for other purposes. This would save us from adding new code to the repository, but it would still use an unnecessary dependency. I wonder about your opinion.
Nice find, I did not know about that before. For this purpose I think it's better to not add dependency as it would save time in the future dealing with dependabot. I don't think the added new code will need a lot of maintenance, and it's also only used in one small part of the app. What do you think? |
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.
For this purpose I think it's better to not add dependency as it would save time in the future dealing with dependabot. I don't think the added new code will need a lot of maintenance, and it's also only used in one small part of the app. What do you think?
I agree. Let’s proceed with adding this PR. I’ve approved it, but I won’t merge it yet to give you a chance to address the feedback I provided.
I noticed a bug in the current release. We're rounding 1.99 GB to 1 GB. We're also fixing it with this PR. I guess it's not a critical issue and no need to report it to HEs.
before | after | web |
---|---|---|
WooCommerce/src/main/kotlin/com/woocommerce/android/extensions/StringExt.kt
Outdated
Show resolved
Hide resolved
b234134
to
b451d03
Compare
Great catch, thanks for testing so thoroughly! I think it's nothing we should announce, but great that it's fixed as a side effect. |
Description
While looking at the Dependabot PR #13044 , I investigated a little bit and found that the dependency common-io was used just for a function in
WCSSRModelExt
. Then I found from Stack Overflow a function that can replace it without the need for the dependency:https://stackoverflow.com/a/5599842
This PR adds that function and unit tests, replaces the usage, and removes the dependency.
Steps to reproduce
Testing information
Nothing specific needed here.
The tests that have been performed
I checked the steps listed above.
Images/gif
n/a
RELEASE-NOTES.txt
if necessary. Use the "[Internal]" label for non-user-facing changes.Reviewer (or Author, in the case of optional code reviews):
Please make sure these conditions are met before approving the PR, or request changes if the PR needs improvement: