php-http / curl-client

cURL client
http://httplug.io
MIT License
445 stars 28 forks source link

cleanup for PSR-17 #53

Closed dbu closed 5 years ago

dbu commented 5 years ago

@mekras i went over everything again and propose these cleanups. and we now have psr-17 factories, so we can use those.

dbu commented 5 years ago

should we plan on releasing one more 1.x version before merging this and creating 2.0? master already implements the psr-18 client interface. that is not a bc break for consumers, but not sure if it makes sense to have a half-cleaned version...

mekras commented 5 years ago

@dbu, why did you delete PHPDoc?

dbu commented 5 years ago

that is a habit i picked up. in my team, we delete phpdoc when it does not add anything over what is already in the parameter type / return type declarations. i replaced things like @param string $method HTTP method with naming the parameter string $httpMethod, then the phpdoc does not add any additional information.

i also removed all phpdoc on methods when its a copy of the super class / interface phpdoc. tools like phpstorm will show you the base doc in autocomplete, so only having {@inheritdoc} also feels unnecessary.

in my opinion, this makes the code more readable. but i can add some of that back in if you disagree.

mekras commented 5 years ago

If this doesn't go against the rules of the project, I'd rather return the PHPDoc. For example, using {@inheritdoc}, in my opinion, forces the developer to perform extra actions to view the descriptions. I believe that if PHPDoc does not contain anything useful, it is more a reason to improve it than to remove it.

dbu commented 5 years ago

If this doesn't go against the rules of the project, I'd rather return the PHPDoc. For example, using {@inheritdoc}, in my opinion, forces the developer to perform extra actions to view the descriptions.

i don't think we have any established rules for this... afaik we say we follow symfony code style but i would not know how deep that code style discusses such things. i can change to inheritdocs to make it clear to people who work with plain text editors that there is doc somewhere.

I believe that if PHPDoc does not contain anything useful, it is more a reason to improve it than to remove it.

i think in the places where i removed the phpdoc, there is simply not more to say. my take on this is: when there is something to explain, by all means do it and have phpdoc. but __construct usually does not need further explanation what the method does. same for something like array $curlOptions we don't need to say more imho. to me, if anything, the absence of phpdoc helps to see that there is no further explanation. if the phpdoc is the same as the variable name, it takes more time to read and realize that there is no more information.

dbu commented 5 years ago

i added {@inheritdoc} where it made sense.