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.

Sign up for GitHub

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

Jump to bottom

#41 Avoid issues when the package is scheduled for removal #46

Merged
merged 5 commits into from
Sep 6, 2017
Merged

#41 Avoid issues when the package is scheduled for removal #46

merged 5 commits into from
Sep 6, 2017

Conversation

Jean85
Copy link
Contributor

@Jean85 Jean85 commented Sep 4, 2017

This is to solve #41, as proposed in my comment.

Ocramius
Ocramius requested changes Sep 4, 2017
View reviewed changes
Copy link
Owner

@Ocramius Ocramius left a comment

Choose a reason for hiding this comment

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

Overall, seems good. I would suggest simulating a removal via the composer CLI in .travis.ci (additional step)

src/PackageVersions/NotInstalledException.php Outdated

class NotInstalledException extends \RuntimeException
{
const MESSAGE = "Folder of ocramius/package-versions not found, it seems that it's not installed";
Copy link
Owner

Choose a reason for hiding this comment

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

Move to a static named constructor instead

src/PackageVersions/Installer.php Outdated
$composer->getConfig(),
$composer->getPackage()
);
} catch (NotInstalledException $exception) {
Copy link
Owner

Choose a reason for hiding this comment

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

Since you are using private-only API, throwing and catching in the same scope seems like a bad idea (it's basically a GOTO implemented via exceptions).

Can this be re-designed somehow?

@Ocramius Ocramius added the bug label Sep 4, 2017
@Ocramius Ocramius added this to the 1.1.3 milestone Sep 4, 2017
@Jean85
Copy link
Contributor Author

Jean85 commented Sep 5, 2017 •
edited
Loading

I've addressed your concerns in two different commits, so it should be easy to review:

  • I've refactored to avoid the exception altogether
  • I've added a step on the Travis build that does an integration test with composer to avoid regressions on this issue

[EDIT] Crap, broken build, working on it!

@Jean85 Jean85 mentioned this pull request Sep 5, 2017
Merged
@Jean85
Copy link
Contributor Author

Jean85 commented Sep 5, 2017

Ok now build is green! @Ocramius if it's 👍 for you, it's ready to be merged!

For some unknown reason tests were failing due to the presence of the new composer.json for the integration test. I preferred switching to a new E2E test for this scenario, it works well too.

@Ocramius Ocramius assigned Ocramius and unassigned Jean85 Sep 5, 2017
@Ocramius Ocramius changed the title Avoid issues when the package is scheduled for removal #41 Avoid issues when the package is scheduled for removal Sep 6, 2017
@Ocramius Ocramius merged commit 6e4fb4f into Ocramius:master Sep 6, 2017
Ocramius added a commit that referenced this pull request Sep 6, 2017
@Ocramius
#41 #46 adding @group annotation to newly introduced tests
d851b22
Ocramius added a commit that referenced this pull request Sep 6, 2017
@Ocramius
#41 #46 removing trailing whitespace
13a5686
Ocramius added a commit that referenced this pull request Sep 6, 2017
@Ocramius
Merge branch 'fix/#41-#46-avoid-generating-when-package-scheduled-for…
c094b15 
…-removal'

Close #41
Close #46
@Ocramius
Copy link
Owner

Ocramius commented Sep 6, 2017

@Jean85 merged, thanks! 👍

@Ocramius Ocramius mentioned this pull request Sep 6, 2017
Closed
@Jean85 Jean85 deleted the fix-scheduled-for-removal branch September 6, 2017 15:22
@Jean85
Copy link
Contributor Author

Jean85 commented Sep 6, 2017

You're welcome! 😉

@ondrejmirtes
Copy link

@Ocramius @Jean85 Can you both release new versions of your packages so I can require them as minimal in PHPStan's dependencies? :) Thanks.

@Ocramius
Copy link
Owner

Ocramius commented Sep 6, 2017

@ondrejmirtes this was just released: https://github.com/Ocramius/PackageVersions/releases/tag/1.1.3

@ondrejmirtes
Copy link

Good, now @Jean85 should require this version in jean85/pretty-package-versions and release a new version :)

@ondrejmirtes
Copy link

Good, now @Jean85 should require this version in jean85/pretty-package-versions and release a new version :) And then I will release PHPStan 0.8.5 :)

@Ocramius
Copy link
Owner

Ocramius commented Sep 6, 2017

@ondrejmirtes given that jean85/pretty-package-versions works as-is, does @Jean85 really need to make a release? Even with preferred stable dependencies, the last patch version should be allowed.

@ondrejmirtes
Copy link

ondrejmirtes commented Sep 6, 2017 via email

@Jean85
Copy link
Contributor Author

Jean85 commented Sep 6, 2017

No biggies, I can release a patch version too. Doing it right now...

@Ocramius
Copy link
Owner

Ocramius commented Sep 6, 2017

Makes sense, although you really should always have latest/greatest :D

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Ocramius Ocramius Ocramius requested changes

Assignees

@Ocramius Ocramius

Labels
bug
Projects
None yet
Milestone
1.1.3
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@Jean85 @Ocramius @ondrejmirtes