Skip to content

Ignore inaccessible class child nodes #15

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

Merged

Conversation

herndlm
Copy link
Contributor

@herndlm herndlm commented May 2, 2022

Based on the idea that was voiced in szepeviktor/phpstan-wordpress#99 (comment)

It should be safe to not include private constants/properties/methods of classes. And if the class is final, protected ones can be ignored too IMO.

Fyi this only makes the wordpress-stubs around 0.1M smaller, so it won't change much in regard to parsing performance there.

What do you think?

@szepeviktor
Copy link
Member

szepeviktor commented May 2, 2022

What do you think?

I think this could be resolved in PHPStan. BTW PHPStan is written in PHP so it is not expected to be as fast as C code.

The current maintenance effort is over my personal max.

@herndlm
Copy link
Contributor Author

herndlm commented May 2, 2022

the performance problems should be fixed in phpstan for sure. this PR was meant to just clean up the stubs a bit further which is not really related and makes sense anyways IMO. it could also be made configurable with a "include_inaccessible_class_nodes" config (a couple of lines more I already prepared).

but it's also fine to keep it as it is of course, this is your stubs generator :) but maybe you also want to remove the "PR welcome" message from the README then.

@szepeviktor
Copy link
Member

szepeviktor commented May 2, 2022

Martin! We were forced to fork this project as we had problems.
https://github.com/php-stubs/generator/commits/master

Stubs could be modified here: https://github.com/php-stubs/wordpress-stubs/blob/master/visitor.php#L37

I'll merge this as soon as any other person is complaining about private methods.

Okay?

@herndlm
Copy link
Contributor Author

herndlm commented May 2, 2022

  1. if you want to remove a node I think you have to do this via leaveNode instead, see https://github.com/nikic/PHP-Parser/blob/v4.13.2/lib/PhpParser/NodeVisitor.php#L45
  2. sure, that's fine! feel free to keep it open, or also just close it if you want. I also don't have a problem if you say you don't need/want this :)

@szepeviktor
Copy link
Member

szepeviktor commented May 2, 2022

To be honest I do not know what will happen by this PR.
Thousands use WP core stubs!

@szepeviktor
Copy link
Member

szepeviktor commented May 2, 2022

Actually this generator is a highly unstable project.
I do not dare to touch it!!! I'm not a businessman nor a developer, I sit here at home and think.
For me theory is not practice.

You must be brave enough to touch daily work of thousands.

@szepeviktor
Copy link
Member

szepeviktor commented May 2, 2022

@herndlm Have you seen the actual diff?

164 insertions(+), 2959 deletions(-)

@herndlm
Copy link
Contributor Author

herndlm commented May 2, 2022

I briefly scrolled through it, yeah. Was looking fine to me. But please let's not argue over this, if it's too risky then it shall not be touched and over :) or, as you suggested, work can be continued if more people want/need this.

@johnbillion
Copy link
Contributor

I would be in favour of removing these inaccessible nodes by default. Perhaps with a config flag to not exclude them in cases where they are needed?

@johnbillion
Copy link
Contributor

If it breaks anybody's code then they can ask Viktor for a refund.

@szepeviktor
Copy link
Member

with a config flag

Let's do this.

@herndlm herndlm force-pushed the ignore-inaccessible-class-child-nodes branch from e122630 to 0d6feee Compare February 8, 2023 20:01
@herndlm
Copy link
Contributor Author

herndlm commented Feb 8, 2023

rebased on master and added the config change I had apparently lying around since May :D

@herndlm herndlm marked this pull request as draft February 8, 2023 20:09
@szepeviktor
Copy link
Member

szepeviktor commented Feb 8, 2023

Thank you!
Does --help talk about include_inaccessible_class_nodes?

@herndlm herndlm force-pushed the ignore-inaccessible-class-child-nodes branch from 0d6feee to df76371 Compare February 8, 2023 20:15
@herndlm
Copy link
Contributor Author

herndlm commented Feb 8, 2023

I realized that what I pushed was not ready yet, now it is, including the help adaption.

@herndlm herndlm marked this pull request as ready for review February 8, 2023 20:16
@herndlm
Copy link
Contributor Author

herndlm commented Feb 8, 2023

but let me add a test case for that, sorry :)
UPDATE: now I'm ok with it

@szepeviktor
Copy link
Member

Thank you.

@szepeviktor szepeviktor merged commit 9b9067b into php-stubs:master Feb 8, 2023
@szepeviktor
Copy link
Member

@johnbillion
Copy link
Contributor

Nice one

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