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

Bump to PHP 7.1, add typehtins #137

Merged
merged 19 commits into from Nov 30, 2017

Conversation

Projects
None yet
2 participants
@TomasVotruba
Copy link
Contributor

TomasVotruba commented Nov 26, 2017

Why?

Related Changes

  • use PHPUnit 6.0 syntax for @expectedException

@TomasVotruba TomasVotruba changed the title Bump to PHP 7.1, add typehtins [WIP] Bump to PHP 7.1, add typehtins Nov 26, 2017

@TomasVotruba

This comment has been minimized.

Copy link
Contributor

TomasVotruba commented Nov 27, 2017

PR is ready to merge. Fail is caused by coveralls due to decreased constructor coverage

@TomasVotruba TomasVotruba changed the title [WIP] Bump to PHP 7.1, add typehtins Bump to PHP 7.1, add typehtins Nov 28, 2017

@jaapio
Copy link
Member

jaapio left a comment

I'm not sure if I want to remove all @param from the docblock. Somehow it feels wrong to have them for some params and not for others. On the other hand phpdocumentor will understand the non documented params in the future. So it is not an absolute blocking issue for now. On the other hand there is no need for those non documented @param anymore.

I don't know if the ?Class $param = null is required. Is this forced by some setting in php7.1? Or is it just needed to have this because = null is now only a default value?

*/
public function render(Formatter $formatter = null)
public function render(?Formatter $formatter = null): string

This comment has been minimized.

@jaapio

jaapio Nov 29, 2017

Member

Is this not dubble? It already allowed null as a value.

@TomasVotruba

This comment has been minimized.

Copy link
Contributor

TomasVotruba commented Nov 30, 2017

As for nullable, if it doesn't break the used code, it's better to use it since it creates more explicit contract.
See this short answer: https://stackoverflow.com/a/14584208/1348344

As for unused typehtins. Do you agree, it is a comment and it should add a value to the code? If so, I realize it has it's meaning pre-PHP 7.0 typehtins. But now it just old-code that does nothing. It doesn't hurt to have it (like static methods everywhere), there is just no reason to have it at all.

You can see trend in PHP from docs to typehints. In the future, there won't be comment neither for arrays (Type[]) and comments will really only add extra value.

@jaapio

This comment has been minimized.

Copy link
Member

jaapio commented Nov 30, 2017

I do agree with you that the docblocks are redundant now.
No changes are needed. Please rebase your branch so I can merge it.

@jaapio

jaapio approved these changes Nov 30, 2017

@TomasVotruba TomasVotruba force-pushed the TomasVotruba:phpunit-rector branch from 473afbc to f2fff17 Nov 30, 2017

@TomasVotruba

This comment has been minimized.

Copy link
Contributor

TomasVotruba commented Nov 30, 2017

Thank you for understanding! Rebased done

Again only coveralls is failing

@jaapio jaapio merged commit c19b4d1 into phpDocumentor:master Nov 30, 2017

2 of 3 checks passed

coverage/coveralls Coverage decreased (-0.1%) to 93.878%
Details
Scrutinizer 21 new issues, 2 updated code elements
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@TomasVotruba

This comment has been minimized.

Copy link
Contributor

TomasVotruba commented Nov 30, 2017

🎉 🚀

@TomasVotruba TomasVotruba deleted the TomasVotruba:phpunit-rector branch Nov 30, 2017

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