More type hints and return type declarations. #4963
Conversation
weitzman
requested review from
bircher,
claudiu-cristea,
davereid,
DieterHolvoet,
greg-1-anderson,
jonhattan,
pfrenssen and
Sutharsan
as code owners
| @@ -394,7 +394,7 @@ protected function askReferencedBundles(FieldDefinitionInterface $fieldDefinitio | |||
| return $this->io()->askQuestion($question) ?: []; | |||
| } | |||
|
|
|||
| protected function createField(): FieldConfigInterface | |||
There was a problem hiding this comment.
EntityInterface is less specific, this doesn't seem like a good change. Also, why are classes displayed in long notation (full namespace)? Is this a Drush code style convention?
There was a problem hiding this comment.
Thanks for the review. These changes were all done via Rector. I'm not sure why it picks less specific classes sometimes. Also I'm not sure why it uses full class namespaces, and if there is a way to avoid that. Would you have any interest in further refining this PR? If not, do you think it does more good than bad (i.e. we merge it). I could go either way, but I'm not too interested in working on it more.
There was a problem hiding this comment.
I'm not sure why it picks less specific classes sometimes.
Gaps in the logic I'm assuming, Rector isn't always right even though it's constantly improving.
Also I'm not sure why it uses full class namespaces, and if there is a way to avoid that.
Yes, there's a way: using the AUTO_IMPORT_NAMES option. I enabled that option and I disabled the IMPORT_SHORT_CLASSES option, so classes without namespaces (like \DateTime) aren't imported. We can still change that if someone doesn't agree.
Would you have any interest in further refining this PR? If not, do you think it does more good than bad (i.e. we merge it).
I re-ran Rector with the new options, went over the changes and committed the results. I'm not too familiar with the coverage of automatic tests in this repo, but the impacted commands should probably also be manually tested.
No description provided.