Skip to content

Refactor package selection#180

Closed
mpdude wants to merge 2 commits intocomposer:masterfrom
webfactory:refactor-package-selection
Closed

Refactor package selection#180
mpdude wants to merge 2 commits intocomposer:masterfrom
webfactory:refactor-package-selection

Conversation

@mpdude
Copy link
Copy Markdown
Contributor

@mpdude mpdude commented Oct 13, 2014

I find the selectPackages() part very hard to understand or to test.

This PR attempts to break it up into smaller pieces, namely:

  • A PackageSelection class which tracks the packages that are to be processed
  • A LinkResolver which can follow dependencies from the current PackageSelection and add more packages if needed
  • LinkProviders that know which dependencies to follow for a given package
  • FilteredPackageSelection which helps in the case of "filtered builds".

Feedback is welcome, but please let's try to focus on fundamental issues before discussing CS glitches.

Thanks!

@stof
Copy link
Copy Markdown
Contributor

stof commented Oct 13, 2014

It looks like your work is not build on top of the latest Satis version. Github reports that the PR conflicts with master

@mpdude
Copy link
Copy Markdown
Contributor Author

mpdude commented Oct 13, 2014

Thanks, fixed.

@mpdude mpdude changed the title [RFC] Break out/refactor package selection [RFC] Refactor package selection Oct 13, 2014
@naderman
Copy link
Copy Markdown
Member

Yeah I think it's a great idea to refactor this, ideally we'd start having some tests too, if you want to work on those?

@mpdude
Copy link
Copy Markdown
Contributor Author

mpdude commented Nov 20, 2014

Sure, will see what I can do.

@mpdude
Copy link
Copy Markdown
Contributor Author

mpdude commented Nov 20, 2014

While we're at it – what's your minimum requirements for merging this?

It happened to me quite a few times that PRs grew so big that nobody really wanted to review them anymore and they never got merged.

I don't think this changes any behavior right now and we can also add tests in another PR.

@mpdude mpdude changed the title [RFC] Refactor package selection Refactor package selection Nov 20, 2014
@naderman
Copy link
Copy Markdown
Member

On satis? I don't really care about the size. But obviously smaller self contained PRs make sense if you can actually split it up in a meaningful way, but otherwise just keep going here. Only issue will be keeping it in sync with other changes.

@naderman
Copy link
Copy Markdown
Member

In general I'm a bit hesitant with merging something like this right now, not because of the size but because of the lack of tests.

@mpdude
Copy link
Copy Markdown
Contributor Author

mpdude commented Nov 21, 2014

So please help on #184 so we can get the relevant (functional) tests here.

@alcohol
Copy link
Copy Markdown
Member

alcohol commented Feb 19, 2016

Closing this due to lack of activity and conflicting state. Please do feel free to re-open and update this PR if said functionality is still desired. :-)

@alcohol alcohol closed this Feb 19, 2016
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.

4 participants