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

DX: Use prestissimo #403

Merged
merged 3 commits into from
May 5, 2018
Merged

Conversation

keradus
Copy link
Contributor

@keradus keradus commented Apr 2, 2018

The gain won't be big, but I'm showing the approach how #173 shall be done.
If you want it - take it, if not ,close PR, no bad feelings !

.travis.yml Outdated
@@ -22,7 +22,8 @@ before_install:
- wget http://cs.sensiolabs.org/download/php-cs-fixer-v2.phar --output-document=php-cs-fixer.phar

install:
- composer update --prefer-dist --no-interaction $COMPOSER_FLAGS
- composer global show hirak/prestissimo -q || travis_retry composer global require hirak/prestissimo --prefer-dist --no-interaction --no-progress $COMPOSER_FLAGS
Copy link
Contributor Author

Choose a reason for hiding this comment

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

ref #173 (comment)

if prestissimo is already installed, we don't need to install it again.
so same time as project dependencies are cached and prestissimo is not bringing that big benefits, we could cache it as well, so we won't lose any time if it's already there.

so with this change, we have 2 scenarios:

  • no composer cache, install prestissimo plugin and then deps using plugin
  • composer cache is there, don't install plugin and install deps from cache

Copy link
Member

Choose a reason for hiding this comment

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

no need for composer flags here AFAIK, only for the composer update command

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure, I didn't found where that env var was set and what you provide as a value, so I kept it as is. dropping...

.travis.yml Outdated
@@ -22,7 +22,8 @@ before_install:
- wget http://cs.sensiolabs.org/download/php-cs-fixer-v2.phar --output-document=php-cs-fixer.phar

install:
- composer update --prefer-dist --no-interaction $COMPOSER_FLAGS
- composer global show hirak/prestissimo -q || travis_retry composer global require hirak/prestissimo --prefer-dist --no-interaction --no-progress
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add some linebreaks here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

like between this and next install steps, or to split this line? not sure what you mean to be honest.
well, in the end I left allowance to modify my PR to you guys, so feel free to add them yourself however you want to have them!

Copy link
Contributor

Choose a reason for hiding this comment

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

Just to split this line, but yeah I can amend it.

greg0ire
greg0ire previously approved these changes Apr 2, 2018
greg0ire
greg0ire previously approved these changes Apr 2, 2018
.travis.yml Outdated
- |
composer global show hirak/prestissimo --quiet ||
travis_retry composer global require hirak/prestissimo --prefer-dist --no-interaction --no-progress
- composer update --prefer-dist --no-interaction --no-progress $COMPOSER_FLAGS
Copy link
Member

Choose a reason for hiding this comment

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

This composer flags is suspicious, I can't find it defined anywhere :P

Copy link
Contributor

@greg0ire greg0ire Apr 2, 2018

Choose a reason for hiding this comment

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

See 24cf39a , looks like they were never defined at all

Copy link
Contributor Author

Choose a reason for hiding this comment

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

so same as me ;)
if you want to drop it, let us not mix the things and drop it in separate PR please

Copy link
Member

@soullivaneuh soullivaneuh left a comment

Choose a reason for hiding this comment

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

The PR is under conflict because of other merges. Could you please rebase?

@@ -10,7 +10,7 @@ sudo: false

cache:
directories:
- $HOME/.composer/cache/files
- $HOME/.composer
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, caching the whole composer directory looks dangerous to me. Why this is needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

to have prestissimo cached. whole point of this PR.

I am caching whole composer for 3+ years, never had any issues because of that.
Why you find it dangeroues ?

Copy link
Member

Choose a reason for hiding this comment

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

I find it dangerous because we might have not wanted cached file and I already add not wanted dependencies issues before.

But it was a long time ago, let's give a try.

Copy link
Contributor Author

@keradus keradus May 6, 2018

Choose a reason for hiding this comment

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

well, you can put anything in cache, what would be the outcome deps depends only on composer.* files, not cache

@keradus
Copy link
Contributor Author

keradus commented May 5, 2018

conflict resolved

@soullivaneuh soullivaneuh merged commit d12ad3f into sonata-project:master May 5, 2018
@soullivaneuh
Copy link
Member

Thanks!

@keradus keradus deleted the prestissimo branch May 6, 2018 15:50
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.

None yet

4 participants