Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
Replace more docblocks by type-hints#24722
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
ec3b7c0 toe32d019Comparenicolas-grekas commentedOct 28, 2017
kaznovac commentedOct 31, 2017
good initiative, but the parameter documentation should be preserved IMHO |
768004a to55854e7Compare| }catch (\RuntimeException$e) { | ||
| // according to RFC 2616 invalid date formats (e.g. "0" and "-1") must be treated as in the past | ||
| return \DateTime::createFromFormat(DATE_RFC2822,'Sat, 01 Jan 00 00:00:00 +0000'); | ||
| return \DateTime::createFromFormat('U',time() -172800); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This change is required to prevent an integer overflow on 32bit systems.
c27f96a to342ae44Comparenicolas-grekas commentedNov 6, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Status: needs review (enjoy ;-)) Docblocks are really different than actual type hints, their respective accuracies have nothing in common. |
stof commentedNov 7, 2017
We should of course use them from the start for any new API added in 4.1+ |
This PR was merged into the 4.0-dev branch.Discussion----------Replace more docblocks by type-hints| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | no| BC breaks? | yes (but only on constructors and final methods)| Deprecations? | no| Tests pass? | yes| Fixed tickets | -| License | MIT| Doc PR | -Commits-------aaf2265 Replace more docblocks by type-hints
| * @param string $extension | ||
| */ | ||
| privatefunctionupdateValidatorMappingFiles(ContainerBuilder$container,$mapping,$extension) | ||
| privatefunctionupdateValidatorMappingFiles(ContainerBuilder$container,string$mapping,string$extension) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
shouldn't this have void return typehint?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I find void useless, so I didn't add any.
If you want to make it your battle, please do :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Method without void can mean two types of things - either it's return type void, or it's just not filled in or the actual return type is in DocBlock. That's a lot of "or"s :)
For me, it would make sense to add it if it were to be enforced by phpcs (method either has return type hint, or@return docblock). Is there a plan to enforce return typehints?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
no plan I know about
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
| publicfunction__construct(string$inputTimezone =null,string$outputTimezone =null) | ||
| { | ||
| if (null !==$inputTimezone && !is_string($inputTimezone)) { | ||
| thrownewUnexpectedTypeException($inputTimezone,'string'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
this check is useless now as it cannot happen anymore
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
see#24876
| * @expectedException \Symfony\Component\Form\Exception\UnexpectedTypeException | ||
| * @expectedException \TypeError | ||
| */ | ||
| publicfunctiontestValidateDateFormatOption() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
those tests are useless now as they only test php internal behavior
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I'm sure there are more left over, PR is so big...
Additional PR welcomed :)
This PR was merged into the 4.0-dev branch.Discussion----------Remove some unneeded checks/tests| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets | -| License | MIT| Doc PR | -As spotted by@Tobion in#24722 (review)Commits-------05681ec Remove some unneeded checks/tests
TomasVotruba commentedNov 10, 2017
Hey@nicolas-grekas , I'm curious, do you use (at least help of) automated tools for this, like
I plan to add fixer who would take care of thisdeprecated-packages/symplify#416, because it's too time demanding and can be mostly automated, so I look for inspiration. |
nicolas-grekas commentedNov 10, 2017
None, had to stay concentrated a few hours on it, would love a tool to help keep the docblocks low! |
TomasVotruba commentedNov 10, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Holly cow 😲 ! I admire your persistence I did something similar few days ago on composer require symplify/easy-coding-standard --dev # clean-docs.neoncheckers:SlevomatCodingStandard\Sniffs\TypeHints\TypeHintDeclarationSniff:enableEachParameterAndReturnInspection:true vendor/bin/ecs check src --config clean-docs.neon# and to fixvendor/bin/ecs check src --config clean-docs.neon --fix
Feel free to try this and let me know how it works for you. |
TomasVotruba commentedNov 11, 2017
…udaltsov)This PR was merged into the 4.1 branch.Discussion----------[Process] Allow to pass non-string arguments to Process| Q | A| ------------- | ---| Branch? | 4.1| Bug fix? | yes| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets |#28609| License | MIT| Doc PR |Sometimes you might pass integers, floats, nulls as command arguments to the Process constructor.Before 4.0 and#24722 that worked fine. Now it throws because of an invalid type.I think we can drop the type hint here safely and stringify the value implicitly in the method. That will be a little more convenient.Commits-------acf8b83 Remove Process::escapeArgument argument type hint
This PR was merged into the 4.4 branch.Discussion----------Fine tune constructor types| Q | A| ------------- | ---| Branch? | 4.4| Bug fix? | no| New feature? | no <!-- please update src/**/CHANGELOG.md files -->| BC breaks? | no <!-- seehttps://symfony.com/bc -->| Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files -->| Tests pass? | yes <!-- please add some, will be required by reviewers -->| Fixed tickets || License | MIT| Doc PR | symfony/symfony-docs#... <!-- required for new features -->Fine tunes some constructor types that have been added in#24722Form names as integer was only a workaround as forms names are used as array keys which get transformed to int. So it was added as a workaround in#6355 (comment)With typehints added in#24722 those were mostly auto-cast anyway, e.g. in FormBuilder. There are only a few integer form names remaining documented, in the main entry points of the Form component (`\Symfony\Component\Form\FormInterface::add`). Internally it's always a string now. So I could remove some int docs which alsofixes#30032 what@xabbuh tried to do.Some of these changes we're just not done before because of broken tests. It's mainly a missing explicit mock for `TranslationInterface::trans` which returned null. If we had return types hints in interfaces, this wouldn't happen.Commits-------507794a Fine tune constructor types
…e Segatori, tigitz)This PR was squashed before being merged into the 5.0-dev branch (closes#32273).Discussion----------[Process] [5.0] Replace docblocks by type-hints| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | no| BC breaks? | no| Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files -->| Tests pass? | yes <!-- please add some, will be required by reviewers -->| Fixed tickets | continuation of#24722 and checks for#32179| License | MIT| Doc PR | N/AThis PR adds replace docblocks by type hints in the Process component. Some docblocks without valuable information got also removed.Commits-------5c964c5 [Process] [5.0] Replace docblocks by type-hints
…ippe Segatori)This PR was merged into the 5.0-dev branch.Discussion----------[HttpKernel] [5.0] Replace docblocks by type-hints| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | no| BC breaks? | no| Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files -->| Tests pass? | yes <!-- please add some, will be required by reviewers -->| Fixed tickets | continuation of#24722 and checks for#32179| License | MIT| Doc PR | N/AThis PR replaces docblocks by type hints in the HttpKernel component considering#32179. Some docblocks without valuable information got also removed.<!--Replace this notice by a short README for your feature/bugfix. This will help peopleunderstand your PR and can be used as a start for the documentation.Additionally (seehttps://symfony.com/roadmap): - Bug fixes must be submitted against the lowest maintained branch where they apply (lowest branches are regularly merged to upper ones so they get the fixes too). - Features and deprecations must be submitted against branch 4.4. - Legacy code removals go to the master branch.-->Commits-------9e570a2 [Http-Kernel][5.0] Add type-hints
…, Tobion)This PR was squashed before being merged into the 5.0-dev branch (closes#32525).Discussion----------[Intl][5.0] Add parameters type-hints| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | no| BC breaks? | no| Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files -->| Tests pass? | yes <!-- please add some, will be required by reviewers -->| Fixed tickets | continuation of#24722 and checks for#32179| License | MIT| Doc PR | N/AThis PR replaces docblocks by type hints in the Intl component considering#32179. Some docblocks without valuable information got also removed.Commits-------ce79f4b [Intl][5.0] Add parameters type-hints
Uh oh!
There was an error while loading.Please reload this page.