#130: Add default content length to progress bar #286

ndemirca · 2024-04-12T09:26:41Z

Closes #130

coveralls · 2024-04-12T09:30:05Z

Pull Request Test Coverage Report for Build 10665956719

Details

0 of 0 changed or added relevant lines in 0 files are covered.
173 unchanged lines in 4 files lost coverage.
Overall coverage increased (+0.1%) to 64.987%

Files with Coverage Reduction	New Missed Lines	%
com/devonfw/tools/ide/tool/mvn/Mvn.java	8	72.93%
com/devonfw/tools/ide/context/IdeContextConsole.java	11	0.0%
com/devonfw/tools/ide/io/IdeProgressBarConsole.java	36	0.0%
com/devonfw/tools/ide/io/FileAccessImpl.java	118	61.69%

Totals
Change from base Build 10636350223:	0.1%
Covered Lines:	5765
Relevant Lines:	8567

💛 - Coveralls

cli/pom.xml

cli/src/main/java/com/devonfw/tools/ide/io/FileAccessImpl.java

cli/src/test/java/com/devonfw/tools/ide/io/FileAccessImplTest.java

jan-vcapgemini

This fix looks quite solid, thanks. I've added some small CR's. After these, the PR should be ready for review.

cli/src/main/java/com/devonfw/tools/ide/io/FileAccessImpl.java

cli/src/test/java/com/devonfw/tools/ide/io/IdeProgressBarTest.java

…ssBar

jan-vcapgemini

Thank you for your last adjustments. I've added some very small CR's.

cli/src/main/java/com/devonfw/tools/ide/io/FileAccess.java

cli/src/main/java/com/devonfw/tools/ide/io/FileAccessImpl.java

cli/src/test/java/com/devonfw/tools/ide/io/IdeProgressBarTest.java

…ssBar

applied code reformat added missing Javadoc param

…ssBar # Conflicts: # cli/src/main/java/com/devonfw/tools/ide/io/FileAccess.java # cli/src/test/java/com/devonfw/tools/ide/context/AbstractIdeTestContext.java # cli/src/test/java/com/devonfw/tools/ide/io/IdeProgressBarTest.java

…ssBar # Conflicts: # cli/src/test/java/com/devonfw/tools/ide/context/AbstractIdeTestContext.java

added AbstractIdeProgressBar implementing IdeProgressBar moved prepareProgressbar logic to IdeProgressBarConsole replaced stepByOne with stepBy and introduced doStepBy adjusted tests

…ssBar # Conflicts: # cli/src/test/java/com/devonfw/tools/ide/context/AbstractIdeTestContext.java

cli/src/main/java/com/devonfw/tools/ide/io/AbstractIdeProgressBar.java

hohwille · 2024-08-30T09:29:59Z

cli/src/main/java/com/devonfw/tools/ide/io/AbstractIdeProgressBar.java

+
+  @Override
+  public void close() {
+    if (this.currentProgress < this.maxLength) {


handle maxLength < 0 (-1) case

I've added a check for maxLength < 0 which will simply return and not change the progress bar anymore.

hohwille · 2024-08-30T09:30:40Z

cli/src/main/java/com/devonfw/tools/ide/io/FileAccess.java

+   * @param test boolean indicating if this is used in a test or not.
   */
-  void download(String url, Path targetFile);
+  void download(String url, Path targetFile, boolean test);


why do we need this parameter? This seems odd to have in a regular API.

this is a left over from the first draft and should be removed

Removed now.

hohwille · 2024-08-30T09:33:13Z

cli/src/main/java/com/devonfw/tools/ide/io/FileAccessImpl.java

+      long size;
+      if (test) {
+        size = 0L;
+      } else {
+        size = source.toFile().length();
+      }


if you need to test this somehow, extract to protected method long getFileSize(Path) create a subclass of FileAccessImpl for test that overrides and returns 0 instead. Then in your JUnit replace the real FileAccess with the test instance... Seems a little bit more complicated but adding this test flag to our official API seems odd to me.

I've implemented this as you've suggested, thanks.

hohwille · 2024-08-30T09:37:03Z

cli/src/main/java/com/devonfw/tools/ide/io/FileAccessImpl.java

+  private void informAboutSettingDefaultContentLength(long contentLength, String url, Path path) {
+
+    String source;
+    if (contentLength == 0) {


why are you using 0 for undefined? IMHO -1 would make more sense also here...

Files can be 0 bytes large and then the user should not get this warning for every file that was empty but gets copied.
Also if the file size is know but smaller than e.g. 1k we should not give a progress bar.
What is good is that we have a dedicated method copyFileWithProgressBar for this use-case so we do not do this for every file. However, I assume this is used when we get a tool release package from a custom tool repository using a file URL and that can also be a network mount that is slower so already copying 1MB could take some seconds...

I've set the default for undefined to -1 now.

cli/src/main/java/com/devonfw/tools/ide/io/IdeProgressBarConsole.java

…Bar.java Co-authored-by: Jörg Hohwiller <[email protected]>

…le.java Co-authored-by: Jörg Hohwiller <[email protected]>

…ssBar

set size for unknown length to -1 instead of 0 added FileAccessTestImpl (for new getFileSize mock returning always -1) added getFileSize to FileAccessImpl removed test param from download and download*WithProgressBar methods added check for unknown size to close method

jan-vcapgemini · 2024-09-02T13:48:36Z

I'm closing this PR and continuing it in #575 for a fresh team review and less cluttering.

ndemirca added 2 commits April 12, 2024 11:09

Added default content length to progress bar

a6799be

added test data

be68571

ndemirca marked this pull request as draft April 12, 2024 09:34

ndemirca added 2 commits April 12, 2024 14:38

Finished bug fix

82a521f

improved unittests

72b90ef

ndemirca marked this pull request as ready for review April 12, 2024 12:45

ndemirca requested a review from VinceHeu April 12, 2024 14:57

hohwille reviewed Apr 12, 2024

View reviewed changes

cli/pom.xml Outdated Show resolved Hide resolved

hohwille reviewed Apr 12, 2024

View reviewed changes

cli/src/main/java/com/devonfw/tools/ide/io/FileAccessImpl.java Outdated Show resolved Hide resolved

hohwille reviewed Apr 12, 2024

View reviewed changes

cli/src/main/java/com/devonfw/tools/ide/io/FileAccessImpl.java Outdated Show resolved Hide resolved

hohwille reviewed Apr 12, 2024

View reviewed changes

cli/src/main/java/com/devonfw/tools/ide/io/FileAccessImpl.java Outdated Show resolved Hide resolved

removed outdated JUnit dependency

7c81d4a

hohwille reviewed Apr 12, 2024

View reviewed changes

cli/src/test/java/com/devonfw/tools/ide/io/FileAccessImplTest.java Outdated Show resolved Hide resolved

VinceHeu removed their request for review April 15, 2024 09:30

used wiremock for the unit tests

2651706

ndemirca marked this pull request as draft April 15, 2024 14:43

jan-vcapgemini requested changes Apr 15, 2024

View reviewed changes

jan-vcapgemini added the bugfix label Apr 15, 2024

jan-vcapgemini added this to the release:2024.04.001 milestone Apr 15, 2024

ndemirca added 2 commits April 16, 2024 11:59

Implemented change requests

301b5bb

added improvements

483311d

ndemirca marked this pull request as ready for review April 16, 2024 15:06

Merge branch 'main' into bug/130-Add-Default-Content-Length-To-Progre…

c4e6d0a

…ssBar

ndemirca requested a review from jan-vcapgemini April 16, 2024 15:20

tobka777 assigned hohwille Apr 17, 2024

tobka777 requested a review from hohwille April 17, 2024 08:33

jan-vcapgemini requested changes Apr 17, 2024

View reviewed changes

ndemirca and others added 2 commits April 17, 2024 12:28

implemented change requests

a95ac8d

Merge branch 'main' into bug/130-Add-Default-Content-Length-To-Progre…

1729d2e

…ssBar

jan-vcapgemini approved these changes Apr 29, 2024

View reviewed changes

ndemirca added 3 commits May 15, 2024 13:43

line ending changes

9886857

Merge branch 'main' into bug/130-Add-Default-Content-Length-To-Progre…

368f3a1

…ssBar

fixed compile issue

2526187

ndemirca marked this pull request as draft May 16, 2024 11:02

Merge branch 'main' into bug/130-Add-Default-Content-Length-To-Progre…

d931dff

…ssBar

ndemirca assigned jan-vcapgemini and unassigned ndemirca May 29, 2024

jan-vcapgemini and others added 8 commits July 8, 2024 15:45

Merge branch 'main' into bug/130-Add-Default-Content-Length-To-Progre…

7ac7fa4

…ssBar

Merge branch 'main' into bug/130-Add-Default-Content-Length-To-Progre…

f7205aa

…ssBar

devonfw#130: cleanup

1ecce2b

applied code reformat added missing Javadoc param

devonfw#130: fixed tests and PluginInstaller

50d38c4

Merge branch 'main' into bug/130-Add-Default-Content-Length-To-Progre…

8dccee1

…ssBar # Conflicts: # cli/src/test/java/com/devonfw/tools/ide/context/AbstractIdeTestContext.java

devonfw#130: refactored progress bar

acf8e96

added AbstractIdeProgressBar implementing IdeProgressBar moved prepareProgressbar logic to IdeProgressBarConsole replaced stepByOne with stepBy and introduced doStepBy adjusted tests

Merge branch 'main' into bug/130-Add-Default-Content-Length-To-Progre…

ba27c39

…ssBar # Conflicts: # cli/src/test/java/com/devonfw/tools/ide/context/AbstractIdeTestContext.java

hohwille reviewed Aug 30, 2024

View reviewed changes

cli/src/main/java/com/devonfw/tools/ide/io/AbstractIdeProgressBar.java Outdated Show resolved Hide resolved

hohwille reviewed Aug 30, 2024

View reviewed changes

cli/src/main/java/com/devonfw/tools/ide/io/IdeProgressBarConsole.java Outdated Show resolved Hide resolved

jan-vcapgemini and others added 4 commits September 2, 2024 10:46

Update cli/src/main/java/com/devonfw/tools/ide/io/AbstractIdeProgress…

f3ccb0a

…Bar.java Co-authored-by: Jörg Hohwiller <[email protected]>

Update cli/src/main/java/com/devonfw/tools/ide/io/IdeProgressBarConso…

df01ffe

…le.java Co-authored-by: Jörg Hohwiller <[email protected]>

Merge branch 'main' into bug/130-Add-Default-Content-Length-To-Progre…

5a146b1

…ssBar

jan-vcapgemini marked this pull request as ready for review September 2, 2024 11:08

jan-vcapgemini mentioned this pull request Sep 2, 2024

#130: progressbar unknown file size #575

Merged

jan-vcapgemini closed this Sep 2, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#130: Add default content length to progress bar #286

#130: Add default content length to progress bar #286

ndemirca commented Apr 12, 2024

coveralls commented Apr 12, 2024 •

edited

Loading

jan-vcapgemini left a comment

jan-vcapgemini left a comment

hohwille Aug 30, 2024

jan-vcapgemini Sep 2, 2024

hohwille Aug 30, 2024

ndemirca Aug 30, 2024

jan-vcapgemini Sep 2, 2024

hohwille Aug 30, 2024

jan-vcapgemini Sep 2, 2024

hohwille Aug 30, 2024 •

edited

Loading

jan-vcapgemini Sep 2, 2024

jan-vcapgemini commented Sep 2, 2024

#130: Add default content length to progress bar #286

#130: Add default content length to progress bar #286

Conversation

ndemirca commented Apr 12, 2024

coveralls commented Apr 12, 2024 • edited Loading

Pull Request Test Coverage Report for Build 10665956719

Details

💛 - Coveralls

jan-vcapgemini left a comment

Choose a reason for hiding this comment

jan-vcapgemini left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hohwille Aug 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jan-vcapgemini commented Sep 2, 2024

coveralls commented Apr 12, 2024 •

edited

Loading

hohwille Aug 30, 2024 •

edited

Loading