Display information when downloading a file. by FlorentTorregrosa · Pull Request #65 · drupal-composer/drupal-scaffold

FlorentTorregrosa · 2017-05-25T15:38:01Z

Currently when downloading files, the command line displays a concatenated string of "Downloading: 100%" which is not helpful to see which file is downloaded where and which file fails downloading in case of failure.

Here is some code to improve the output of the command line.

Thanks for the review.

T2L · 2017-06-29T10:27:01Z

Also, it would be great if we can respect --no-progress composer option and skip showing this information at all.

T2L · 2017-06-29T10:32:08Z

I've checked the output. I my opinion it generates too much noise (and new lines). Suggestions:

combine into a single line (as composer does with vendor dependencies)
add colors

webflo · 2017-06-30T21:32:50Z

Hi, i think the proposed changes by @T2L are valid. @FlorentTorregrosa do you want to change the PR accordingly?

FlorentTorregrosa · 2017-07-01T07:09:06Z

Hello,

Thanks both for your feedback. I will try to change the pull request accordingly. I also think that it would be nice to add colors and to do as composer does, I currently don't know how to do that. That's why I didn't do that before. I will search for that.

I don't know when I would have time to update my PR.

Also if I can have a feedback on #58 because all concerns where addressed, it would be nice so if something needs to be changed I will do it when I will (re-)dig into composer.

FlorentTorregrosa · 2017-08-09T15:39:18Z

Hello,

I have updated the pull request with the suggestions and fixed the tests.

Is it ok now to be merged?

webflo · 2017-08-09T20:05:26Z

    array_walk($this->filenames, function ($filename) use ($version, $destination, &$requests) {
      $url = $this->getUri($filename, $version);
      $this->fs->ensureDirectoryExists($destination . '/' . dirname($filename));
-      $requests[] = new CopyRequest($url, $destination . '/' . $filename, false, $this->io, $this->config);


I think, we can skip this hunk and add the information in line 65.

@webflo Thanks for your review.
At line 65, the $filename variable is not defined. So should I remove lines 40 to 49 and use the previous line 46? And leave line 65 as it is?

Seen with @greg-1-anderson. Will update the pull request.

FlorentTorregrosa · 2017-09-29T12:05:07Z

Saw with @greg-1-anderson, I will refactor FileFetcher, InitialFileFetcher and PrestissimoFileFetcher to avoid code duplication and to use Prestissimo for initial file fetching.

clemens-tolboom · 2017-10-04T13:23:39Z

I was about to submit a PR way worse then this one. 👍

I learned drupal-scaffold is fetching the d.o. files and it's good to know these are fetched and when refetching with composer run-script 'drupal-scaffold'.

FlorentTorregrosa · 2017-10-07T15:28:31Z

Hello,

The refactoring has been done. Thanks for the review.

clemens-tolboom · 2017-10-10T13:02:20Z

+  public function fetch($version, $destination, $erase) {
+    foreach ($this->filenames as $sourceFilename => $filename) {
+      $target = "$destination/$filename";
+      if ($erase || !file_exists($target)) {


So this does not fetch a file when already existing.

That sounds bad to me. Ie a new default.settings.php or .htaccess fixing a security issue I definitely want that downloaded when updating a project.

It fetches a file even if it already exists because in https://github.com/drupal-composer/drupal-scaffold/pull/65/files#diff-bb433a78829fe2e46d711575194d4f50R140 we use the $erase parameter to TRUE and only $erase to FALSE when using it for the initial fetch https://github.com/drupal-composer/drupal-scaffold/pull/65/files#diff-bb433a78829fe2e46d711575194d4f50R143

In the README.md:
The initial hash lists files that should be copied over only if they do not exist in the destination. The key specifies the path to the source file, and the value indicates the path to the destination file.

I was referring @ the code lines changed. I should have dug deeper. Sorry

No problem. Thanks for the review.

…-progress option into account.

…ogress option.

…n if not in verbose mode.

FlorentTorregrosa · 2017-12-09T10:14:51Z

Hello,

I have rebased my changes to followup the last code changes in drupal-scaffold.

Would it be possible to have a review please?

webflo · 2017-12-23T21:55:41Z

The $erase parameter is new feature? It was don't there before.

FlorentTorregrosa · 2017-12-24T09:39:19Z

Hello,

The $erase is not a new feature, it was introduced to refactor the FileFetcher and the initialFileFetcher classes. Therefore the initial fetch also benefits from the PrestissimoFileFetcher.

webflo · 2017-12-24T12:55:41Z

Alright, i understand it has been a while since i looked at the code. Lets rename it to $override? I thinks this is a better fit.

FlorentTorregrosa · 2017-12-24T15:43:16Z

OK. I have changed the name of the variable.

webflo · 2017-12-25T10:44:37Z

Merged.

@FlorentTorregrosa thanks 👍

FlorentTorregrosa · 2017-12-25T11:20:15Z

\o/ Thanks!!! Finally merged 🎁 🎄 🎁. It's Christmas magic :D

Next steps:

webflo reviewed Aug 9, 2017

View reviewed changes

clemens-tolboom reviewed Oct 11, 2017

View reviewed changes

FlorentTorregrosa added 7 commits December 9, 2017 10:51

Display information when downloading a file.

9a47399

Fix tests.

96a1726

Refactor message on signle lines and using colors. Also take the --no…

e424668

…-progress option into account.

Fix fatal error when executing a command that does not have the no-pr…

57ccee5

…ogress option.

Prestissimo: do not display twice download infos and display info eve…

d35b2bb

…n if not in verbose mode.

Standardize the output with the Composer output.

7ec4159

Refactor fetchers to have initial file using prestissimo if available.

027cc50

FlorentTorregrosa force-pushed the download_info branch from 3f1a173 to 027cc50 Compare December 9, 2017 10:06

drupal-composer deleted a comment from FlorentTorregrosa Dec 24, 2017

Rename $erase variable into $override.

c4f4ddb

webflo merged commit 215ffb3 into drupal-composer:master Dec 25, 2017

FlorentTorregrosa deleted the download_info branch December 25, 2017 11:20

Conversation

FlorentTorregrosa commented May 25, 2017

Uh oh!

T2L commented Jun 29, 2017

Uh oh!

T2L commented Jun 29, 2017

Uh oh!

webflo commented Jun 30, 2017

Uh oh!

FlorentTorregrosa commented Jul 1, 2017

Uh oh!

FlorentTorregrosa commented Aug 9, 2017

Uh oh!

webflo Aug 9, 2017

Choose a reason for hiding this comment

Uh oh!

FlorentTorregrosa Aug 10, 2017

Choose a reason for hiding this comment

Uh oh!

FlorentTorregrosa Sep 29, 2017

Choose a reason for hiding this comment

Uh oh!

FlorentTorregrosa commented Sep 29, 2017

Uh oh!

clemens-tolboom commented Oct 4, 2017

Uh oh!

FlorentTorregrosa commented Oct 7, 2017

Uh oh!

clemens-tolboom Oct 10, 2017

Choose a reason for hiding this comment

Uh oh!

FlorentTorregrosa Oct 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

clemens-tolboom Oct 12, 2017

Choose a reason for hiding this comment

Uh oh!

FlorentTorregrosa Oct 12, 2017

Choose a reason for hiding this comment

Uh oh!

FlorentTorregrosa commented Dec 9, 2017

Uh oh!

webflo commented Dec 23, 2017

Uh oh!

FlorentTorregrosa commented Dec 24, 2017

Uh oh!

webflo commented Dec 24, 2017

Uh oh!

FlorentTorregrosa commented Dec 24, 2017

Uh oh!

webflo commented Dec 25, 2017

Uh oh!

FlorentTorregrosa commented Dec 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

FlorentTorregrosa Oct 11, 2017 •

edited

Loading

FlorentTorregrosa commented Dec 25, 2017 •

edited

Loading