Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Take a look at that needs-refactoring! #103

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from

Conversation

szepeviktor
Copy link
Contributor

@szepeviktor szepeviktor commented Sep 26, 2020

From #98

@szepeviktor
Copy link
Contributor Author

szepeviktor commented Sep 26, 2020

Cure

Sending commits ....

@szepeviktor szepeviktor marked this pull request as ready for review September 26, 2020 16:02
@szepeviktor
Copy link
Contributor Author

szepeviktor commented Sep 26, 2020

@chesio Done 🍏

Please prefer the new list<bla> notation instead the old bla[]

@chesio
Copy link
Owner

chesio commented Sep 28, 2020

Hi @szepeviktor,

Thanks for the PR.

Generics are completely new stuff for me, do you have some online resources you would recommend to read?

I'm not sure about including the WP_List_Table stub - seems a bit overkill for me to "duplicate" entire class just to be able to increase test level.

@szepeviktor
Copy link
Contributor Author

szepeviktor commented Sep 28, 2020

Generics are completely new stuff for me, do you have some online resources you would recommend to read?

See the link on the word "generics" :)

seems a bit overkill for me to "duplicate" entire class

  1. it contains no code, it is a stub
  2. you don't have to distribute it through Composer/GitHub ZIP
  3. there is only 1 other way to make $item an array vs. object in core's docblocks: write a lot of PHPStan code to force types...

@szepeviktor
Copy link
Contributor Author

Please take a look at what PHPStan level is the default for this tiny project: https://github.com/szepeviktor/unique-email-address

@szepeviktor
Copy link
Contributor Author

szepeviktor commented Aug 17, 2023

@chesio Friendly ping 🏓

(from the far away past)

@chesio
Copy link
Owner

chesio commented Aug 17, 2023

@szepeviktor,

thanks for the ping, I might have time to review it tomorrow.

@szepeviktor szepeviktor mentioned this pull request Feb 7, 2024
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.

2 participants