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

Improve usage example and tests #1933

Merged
merged 6 commits into from
Aug 11, 2023
Merged

Improve usage example and tests #1933

merged 6 commits into from
Aug 11, 2023

Conversation

hgraceb
Copy link
Contributor

@hgraceb hgraceb commented Aug 10, 2023

  • Improve tests.
  • Improve usage example for Dio.download.
  • Remove unused code.
  • Fix typo in example/lib/download_with_trunks.
  • Replace the broken resource: http://download.dcloud.net.cn/HBuilder.9.0.2.macosx_64.dmg.
  • Disable the compression to assure the value of total argument of onReceiveProgress is not -1 in example/lib/download.

New Pull Request Checklist

  • I have read the Documentation
  • I have searched for a similar pull request in the project and found none
  • I have updated this branch with the latest main branch to avoid conflicts (via merge from master or rebase)
  • I have added the required tests to prove the fix/feature I'm adding
  • I have updated the documentation (if necessary)
  • I have run the tests without failures
  • I have updated the CHANGELOG.md in the corresponding package

Additional context and info (if any)

There was no warning previously due to the default dynamic parameter type.

@hgraceb hgraceb requested a review from a team as a code owner August 10, 2023 04:45
Comment on lines 231 to 235
/// onReceiveProgress: (received, total) {
/// if (total != -1) {
/// print((received / total * 100).toStringAsFixed(0) + '%');
/// print('${(received / total * 100).toStringAsFixed(0)}%');
/// }
/// },
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// onReceiveProgress: (received, total) {
/// if (total != -1) {
/// print((received / total * 100).toStringAsFixed(0) + '%');
/// print('${(received / total * 100).toStringAsFixed(0)}%');
/// }
/// },
/// onReceiveProgress: (received, total) {
/// if(total < 1) return;
/// final percentage = (received / total * 100).toStringAsFixed(0)
/// print('$percentage%');
/// },

Comment on lines 55 to 57
if (total != -1) {
print((received / total * 100).toStringAsFixed(0) + '%');
print('${(received / total * 100).toStringAsFixed(0)}%');
}
Copy link
Contributor

@ueman ueman Aug 10, 2023

Choose a reason for hiding this comment

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

Suggested change
if (total != -1) {
print((received / total * 100).toStringAsFixed(0) + '%');
print('${(received / total * 100).toStringAsFixed(0)}%');
}
if (total < 1) return;
final percentage = (received / total * 100).toStringAsFixed(0)
print('$percentage%');

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  • I replaced 1 with 0 because 0 is the actual boundary value.
  • I merged the definition and the print statement together.
-  if (total < 1) return;
-  final percentage = (received / total * 100).toStringAsFixed(0)
-  print('$percentage%');
+  if (total <= 0) return;
+  print('percentage: ${(received / total * 100).toStringAsFixed(0)}%');

Copy link
Contributor

Choose a reason for hiding this comment

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

I replaced 1 with 0 because 0 is the actual boundary value.

That's fine for me.

I merged the definition and the print statement together.

I believe that's harder to read, but since no one else complained, it seems like that's a me problem :D

@hgraceb hgraceb changed the title Improve usage example for Dio.download Improve usage example and tests Aug 10, 2023
},
);
}

/// Downloading by spiting as file in chunks
Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch!

@ueman ueman added this pull request to the merge queue Aug 11, 2023
Merged via the queue into cfug:main with commit e04b791 Aug 11, 2023
27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants