Skip to content

PhpSpec 4 #10

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

Merged
merged 4 commits into from
Oct 17, 2017
Merged

PhpSpec 4 #10

merged 4 commits into from
Oct 17, 2017

Conversation

Sam-Burns
Copy link
Contributor

No description provided.

@alexcarol
Copy link

Is there any blocker for this PR to be merged? How can I help?

"php": "~5.6||~7.0",
"phpspec/phpspec": "~3.0",
"php": "^7.0",
"phpspec/phpspec": "^4.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

What about "phpspec/phpspec": "~3.0||~4.0"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are previous releases which already cater to PhpSpec 3 - maybe you could use those if you can't upgraded to PhpSpec 4?

Copy link
Member

Choose a reason for hiding this comment

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

@Landerstraeten the update would be for PHPSpec v4. Allowing both versions would cause problems when people try to install updated version with 3.0 versions (and we can only make sure it's compatible with v4). After this PR is merged, the composer will be able to detect which version of phpspec-code-coverage to install depending on which PhpSpec version is used. For PhpSpec v3, the older version of the package that supports PhpSpec v3 will be used.

@@ -24,12 +24,12 @@
"docs": "https://github.com/leanphp/phpspec-code-coverage#phpspec-code-coverage"
},
"require": {
"php": "~5.6||~7.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it should be kept like this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The PR switches to PhpSpec 4, which only works in PHP 7

Copy link
Member

Choose a reason for hiding this comment

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

The update is correct. For anyone, who will use PHP 5.x, the composer will use the older version of this package (the one before this PR is merged).

@QuingKhaos
Copy link

Hey there, any blockers on this? 😃

@roukmoute
Copy link
Contributor

Any news about this P.R.?

Copy link
Contributor

@roukmoute roukmoute left a comment

Choose a reason for hiding this comment

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

You have forgotten to update branch-alias

@JurRutten
Copy link

@Sam-Burns please check the comments on this pr...
Let's get this stuff out the door :) 👍

Copy link
Contributor

@DavidGarciaCat DavidGarciaCat left a comment

Choose a reason for hiding this comment

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

Hey there,

Nice to see this PR, hope we can see it merged soon.

Just 1 question please.

Thanks,

@@ -22,7 +22,7 @@ package with compatible version numbers for stable releases.
- PHP 5.6+ or PHP 7+
- [Xdebug][3] or [phpdbg][4] extension enabled (PHP 7+ is required for code
generation to work with [phpdbg][4]).
- [PhpSpec v3][2]
- [PhpSpec v3 or v4][2]
Copy link
Contributor

Choose a reason for hiding this comment

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

If composer.json file has this change:

-        "phpspec/phpspec": "~3.0",
+        "phpspec/phpspec": "^4.0",

Then why do we need to keep PhpSpec v3?

Choose a reason for hiding this comment

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

Looks like a good remark, I don't think we need this anymore

Copy link
Member

Choose a reason for hiding this comment

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

The composer.json only refers to the latest version, which only supports phpspec 4.0. Composer will be able to pick up an older version of the package appropriately. For now, as long as PhpSpec3 is still used in various projects, I believe it is correct to note that extension supports both versions of PhpSpec.

@@ -24,12 +24,12 @@
"docs": "https://github.com/leanphp/phpspec-code-coverage#phpspec-code-coverage"
},
"require": {
"php": "~5.6||~7.0",
"phpspec/phpspec": "~3.0",
"php": "^7.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

Question: Is PHP 7.1 working with PHP Spec v4?

Choose a reason for hiding this comment

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

Yes, it works with 7.1

Copy link
Contributor

Choose a reason for hiding this comment

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

Then I suggest to add it too, just to make sure it won't be a problem for some projects in case they have new criteria like the minimum PHP version

Copy link
Member

Choose a reason for hiding this comment

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

The ^7.0 should be specific enough as it will allow any 7.x versions to be used. The ^ means not earlier version than.

@roukmoute
Copy link
Contributor

Please do not forget my previous comment.

@ek9
Copy link
Member

ek9 commented Oct 16, 2017

I have reviewed the comments from everyone and I believe the changes should be good for merging. I will do some final tests and prepare separate branch for any maintenance for 3.x. I will test that everything works as it should and this should be merged in the coming day and released as 4.0.0.

Thank you for contribution.

@ek9
Copy link
Member

ek9 commented Oct 17, 2017

Merging, thank you for contribution everyone!

@ek9 ek9 merged commit 330f85c into leanphp:master Oct 17, 2017
This was referenced Oct 17, 2017
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.

8 participants