code improvements, readability enhancements, type safety, modern syntax adaptation #108
Conversation
Interface's method signature: ```php public function getQRCodeImage(string $qrtext, int $size): string; ```
There was a problem hiding this comment.
So I went over the code changes. This is not a refactor like the title implies, but rather a set of changes to make the code use modern syntax. Use match instead of switch, use null coalescing operator, etc... I didn't test it but it should indeed not have any impact on code behavior.
As far as I'm aware, refactoring means introducing changes in an existing codebase in a way that doesn't change the software's behavior but improves some aspects of it, such as readability, performance, etc. |
To me, refactoring means having an impact on the private API, and maybe public, by splitting/merging classes, and mainly doing refactoring of functions, in the algebraic/mathematical sense of the term. But honestly your definition works too and nobody really care. It's just that reading "massive refactoring" I was a bit scared, but in fact it's more like "many small code improvements with no effective changes" ;) |
xHeaven
changed the title
xHeaven
changed the title
|
Generally such 'huge' PR's aren't very helpful, this is so much that it's hard to see what has changed and what hasn't. Next time, please do one thing, make it into a PR and then make a new PR for the next thing. I agree with most, if not all, of @willpower232's 'criticisms' above. I've just returned home from a weekend away; I'll go over it myself soon. That's not to say the PR and your work isn't appreciated, just to be clear! |
|
Appreciate the replies and reviews. I've answered some of them, I'll answer all of them a bit later today.
Absolutely understandable, I wasn't sure if I should put all of these things into a single PR as some kind of all-in-one improvement bundle or open a bunch of PRs instead, and I went with this after all. I'll open more PRs next time. |
|
I've answered all of your reviews and marked most of them as resolved. If you've got a spare 5 minutes sometimes, could you take a look at it? @willpower232 Thanks! |
|
I wasn't aware the PHP CS Fixer config was so extra compared to just the bare standard, I usually just choose to run I think this PR size is fine since its all overlapping stuff and the codebase has aged differently in different places with the evolution of PHP since it was first done. I don't think another PR of this size would exist in this repo for some time! Thanks for all the explanations and indulging my many duplicates, I think I'm happy once the camelCase has happened. |
|
Yeah, the Fixer config has quite a few different rules other than just PSR12, haha. |
See commit messages. As far as I know, there are no breaking changes.