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

Reimplemented PR#232, PR#110 #266

Merged
merged 2 commits into from
Nov 20, 2015
Merged

Reimplemented PR#232, PR#110 #266

merged 2 commits into from
Nov 20, 2015

Conversation

realshadow
Copy link
Contributor

This is reimplemented #232, which also closes #110.

This allows for building a single package by it's url address, e.g.

php bin/satis build satis.json public --repository-url https://github.com/realshadow/serializers.git

Building by specifying both - package and repository-url argument is not possible and should be used separately.

Can someone please look at this?

Thanks

@realshadow realshadow mentioned this pull request Nov 20, 2015
@@ -124,7 +130,13 @@ protected function execute(InputInterface $input, OutputInterface $output)

$composer = $this->getApplication()->getComposer(true, $config);
$packageSelection = new PackageSelection($output, $outputDir, $config, $skipErrors);
$packageSelection->setPackagesFilter($packagesFilter);

if($repositoryUrl !== null) {

Choose a reason for hiding this comment

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

Missing space here.

@realshadow
Copy link
Contributor Author

Good catch @mbrodala , fixed.

@mbrodala
Copy link

Thanks, looks good from my POV.

@alcohol
Copy link
Member

alcohol commented Nov 20, 2015

Thanks :)

alcohol added a commit that referenced this pull request Nov 20, 2015
@alcohol alcohol merged commit 2f01d88 into composer:master Nov 20, 2015
@realshadow realshadow deleted the repository_filter branch November 20, 2015 14:33
@realshadow realshadow restored the repository_filter branch November 20, 2015 14:33
@dol
Copy link

dol commented Nov 20, 2015

@realshadow Well done. I was waiting a long time for this feature. 🎉

@bgaillard
Copy link

Great, partial update will definitely speed up our development process 👍

@mpdude
Copy link
Contributor

mpdude commented Nov 21, 2015

Hooray 🎆

foreach ($repos as $repo) {
foreach ($repos as $key => $repo) {
if ($this->hasRepositoryFilter()) {
$repoConfig = $repo->getRepoConfig();
Copy link

Choose a reason for hiding this comment

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

It's not safe to use the 'getRepoConfig()' method. It's not defined in the RepositoryInterface.
E.g.:
I ran into the problem that the Composer\Repository\PearRepository lacks this method.

Copy link
Contributor

Choose a reason for hiding this comment

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

So does Composer\Repository\ArtifactRepository, I guess. It really lacks of tests ...

Copy link
Member

Choose a reason for hiding this comment

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

Feel free to submit more PR's guys. I'll accept anything, as long as it does not look insane. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Both Pear and Artifact repository lack this method even though they both accept and work with $repoConfig. @alcohol Could this perhaps be implemented in Composer with a PR? Because at the moment I don't know how to get hold of repository url.

Copy link

Choose a reason for hiding this comment

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

I think an additional interface with the method 'getRepoConfig()' would be ideal. In the satis code, it's then possible to check if the repository implements this interface. Meanwhile this could be fixed by checking, if the repo extends the ArrayRepository class.

Copy link
Member

Choose a reason for hiding this comment

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

Seems like a sane patch @realshadow if you wanna submit a PR..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dol, @JamesRezo this issue should be resolved after merging of composer/composer#4638. I can add additional check if repository class implements required interface later tonight, if neccessary.

Sorry for delay :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Great ! :)

Copy link

Choose a reason for hiding this comment

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

@realshadow Don't worry about the delay. I'm happy that we'll be able to speed up our satis build soon. Thank you for your work.

Copy link
Member

Choose a reason for hiding this comment

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

@realshadow don't forget to update the lockfile to depend on the latest version of composer/composer when you submit a new PR (but please limit the update to composer/composer only by running composer update composer/composer). :)

@dol
Copy link

dol commented Nov 23, 2015

Looking at the code I realised that this pull request adds the ability to generate a package.json/index.html that only contains the filtered repository. @bgaillard It's not a fix for a partial update. I was hoping for the same feature.

@realshadow
Copy link
Contributor Author

@dol Do you have a specific use case? It works for me with 10 repo config and rebuilding only one pre at time.

@dol
Copy link

dol commented Nov 23, 2015

@realshadow Sorry, my comment was wrong. Our current setup always generated a new temporary folder and used this folder as build output. If I use a fixed/static folder it works like a charm.

@mathroc
Copy link
Contributor

mathroc commented Nov 25, 2015

thx @realshadow

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

9 participants