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

Display information when downloading a file. #65

Merged
merged 8 commits into from Dec 25, 2017

Conversation

@FlorentTorregrosa
Copy link
Contributor

commented May 25, 2017

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

This comment has been minimized.

Copy link

commented Jun 29, 2017

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

@T2L

This comment has been minimized.

Copy link

commented Jun 29, 2017

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

This comment has been minimized.

Copy link
Member

commented Jun 30, 2017

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

@FlorentTorregrosa

This comment has been minimized.

Copy link
Contributor Author

commented Jul 1, 2017

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

This comment has been minimized.

Copy link
Contributor Author

commented Aug 9, 2017

Hello,

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

Is it ok now to be merged?

@@ -43,7 +37,16 @@ protected function fetchWithPrestissimo($version, $destination) {
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);
if ($this->progress) {

This comment has been minimized.

Copy link
@webflo

webflo Aug 9, 2017

Member

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

This comment has been minimized.

Copy link
@FlorentTorregrosa

FlorentTorregrosa Aug 10, 2017

Author Contributor

@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?

This comment has been minimized.

Copy link
@FlorentTorregrosa

FlorentTorregrosa Sep 29, 2017

Author Contributor

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

@FlorentTorregrosa

This comment has been minimized.

Copy link
Contributor Author

commented Sep 29, 2017

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

This comment has been minimized.

Copy link

commented Oct 4, 2017

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

This comment has been minimized.

Copy link
Contributor Author

commented Oct 7, 2017

Hello,

The refactoring has been done. Thanks for the review.

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

This comment has been minimized.

Copy link
@clemens-tolboom

clemens-tolboom Oct 11, 2017

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.

This comment has been minimized.

Copy link
@FlorentTorregrosa

FlorentTorregrosa Oct 11, 2017

Author Contributor

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.

This comment has been minimized.

Copy link
@clemens-tolboom

clemens-tolboom Oct 12, 2017

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

This comment has been minimized.

Copy link
@FlorentTorregrosa

FlorentTorregrosa Oct 12, 2017

Author Contributor

No problem. Thanks for the review.

@FlorentTorregrosa FlorentTorregrosa force-pushed the FlorentTorregrosa:download_info branch from 3f1a173 to 027cc50 Dec 9, 2017

@FlorentTorregrosa

This comment has been minimized.

Copy link
Contributor Author

commented Dec 9, 2017

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

This comment has been minimized.

Copy link
Member

commented Dec 23, 2017

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

@FlorentTorregrosa

This comment has been minimized.

Copy link
Contributor Author

commented Dec 24, 2017

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

This comment has been minimized.

Copy link
Member

commented Dec 24, 2017

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

This comment has been minimized.

Copy link
Contributor Author

commented Dec 24, 2017

OK. I have changed the name of the variable.

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

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@webflo

This comment has been minimized.

Copy link
Member

commented Dec 25, 2017

Merged.

@FlorentTorregrosa thanks 👍

@FlorentTorregrosa

This comment has been minimized.

Copy link
Contributor Author

commented Dec 25, 2017

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

Next steps:

@FlorentTorregrosa FlorentTorregrosa deleted the FlorentTorregrosa:download_info branch Dec 25, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.