search

makinacorpus / DbToolsBundle

A PHP library to backup, restore and anonymize databases
https://dbtoolsbundle.readthedocs.io
MIT License
181 stars 15 forks source link

feat: apply rector rules for return types #122

Closed shakaran closed 8 months ago

shakaran commented 8 months ago

This PR helps with future PHP 8+ releases and return types.

Applied rector rules: ReturnTypeFromReturnNewRector ReturnTypeFromReturnDirectArrayRector ReturnTypeFromStrictBoolReturnExprRector ReturnTypeFromStrictConstantReturnRector ReturnTypeFromStrictNativeCallRector

pounard commented 8 months ago

We already use php-cs-fixer and we fine tune our own rules when necessary, we do not use Rector at the moment and didn't planned to do so.

Opening pull requests without any explanation, that only applies a static analysis automatic fix based upon a set of rules that don't match our own convention is bold, and quite confusing.

I don't think see this PR as useful, I'm sorry, but it doesn't fix any bugs nor implement any new features.

shakaran commented 8 months ago

We already use php-cs-fixer and we fine tune our own rules when necessary, we do not use Rector at the moment and didn't planned to do so.

Opening pull requests without any explanation, that only applies a static analysis automatic fix based upon a set of rules that don't match our own convention is bold, and quite confusing.

I don't think see this PR as useful, I'm sorry, but it doesn't fix any bugs nor implement any new features.

I am not adding rector as dependency as you can see, it is just another tool for me for improve code. But both are pretty useful in a lot projects (at same time)

I just apply in my local rector in composer dev mode and sent the PR with the basic rules for PHP 8.3 (without commit the composer.json or composer.lock), even I sent it in small batch of rules to review easy for the project. As far I understand, it is good to have upgrades to new PHP versions supported and also dependencies. Let me know if you are not agree with that too, so I will not spent my time too.

And sorry if you need more deep explanations, I am only just trying to help for this kind of problems in Symfony 7.0 and upgrades to PHP 8.3 image but reading that the work is not useful, it really offends me.

if my help is not welcome, maybe I just have to fork and apply my own repo keeping my own criteria if does not match with this project.

php-cs-fixer is pretty good, but it doesn't cover all upgrades, and more important, rector allows to do automatic refactor even for BC in your own code or define own rules (also php-cs-fixer).

I hope that I can send more PRs are they are at least welcome, I dont really want mantain another project for just a few changes of compabitility

pounard commented 8 months ago

@shakaran I am sorry if I was rude myself, any how your PR was not explicit about what it resolved from the start, it's great that you took the time to explain a bit more, thanks.

shakaran commented 8 months ago

@shakaran I am sorry if I was rude myself, any how your PR was not explicit about what it resolved from the start, it's great that you took the time to explain a bit more, thanks.

ok, no prob. Which more it is needed for get the merge? I would like to send a few more in the following hours if I have time

SimonMellerin commented 8 months ago

Thanks for your work ;)