Skip to content

Refactor get_sites() extension to improve readability#158

Merged
szepeviktor merged 3 commits intoszepeviktor:masterfrom
IanDelMar:get-sites-refactor
Feb 26, 2023
Merged

Refactor get_sites() extension to improve readability#158
szepeviktor merged 3 commits intoszepeviktor:masterfrom
IanDelMar:get-sites-refactor

Conversation

@IanDelMar
Copy link
Copy Markdown
Contributor

Refactors the dynamic function return type extension for get_sites() to improve readability.

Last PR addressing this extension 😇

Comment thread src/GetSitesDynamicFunctionReturnTypeExtension.php Outdated
@szepeviktor
Copy link
Copy Markdown
Owner

Hello good-old pass-by-ref :(
This is sooo PHP4!

Please use properties for passing data around.

@IanDelMar
Copy link
Copy Markdown
Contributor Author

@szepeviktor I switched to properties and now 15 assertions fail. 😞 I'll need some time to figure out what's going on.

@herndlm
Copy link
Copy Markdown
Contributor

herndlm commented Feb 26, 2023

Properties add state to those classes making them mutable. Could be that one line of analysed code sets a property that is then incorrectly used for another line of analysed code?

@IanDelMar
Copy link
Copy Markdown
Contributor Author

I replaced $count with $this->fields... 😆

@szepeviktor szepeviktor merged commit 50153a1 into szepeviktor:master Feb 26, 2023
@IanDelMar IanDelMar deleted the get-sites-refactor branch February 26, 2023 14:03
@herndlm
Copy link
Copy Markdown
Contributor

herndlm commented Feb 26, 2023

Just one more thing though. A get* method that doesn't return anything but changes some state is one of the things that keeps me from sleeping at night xD

Most likely the simplest and safest thing is just to return the relevant values as an array and type it for phpstan with an array shape. You can use it in a sane way via e.g. [$foo, $bar] = $this->getSomething(). Assuming that Viktor isn't still supporting php and WordPress from 10 years ago here xD

@herndlm
Copy link
Copy Markdown
Contributor

herndlm commented Feb 26, 2023

OK too slow 😅 as long as it works

@szepeviktor
Copy link
Copy Markdown
Owner

szepeviktor commented Feb 26, 2023

that keeps me from sleeping at night

Mine is get_template_part that is really print_template_part.

@herndlm
Copy link
Copy Markdown
Contributor

herndlm commented Feb 26, 2023

Oh no, stop it...

@szepeviktor
Copy link
Copy Markdown
Owner

@IanDelMar
Copy link
Copy Markdown
Contributor Author

@herndlm you're right. I'll rename the method when I remove the query string argument support (#159).

@herndlm
Copy link
Copy Markdown
Contributor

herndlm commented Feb 26, 2023

Viktor stop trying to sell me your passive aggressive empty repo 😅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants