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

Define layers based on doc tags #1326

Merged
merged 2 commits into from
Jan 22, 2024

Conversation

brightbyte
Copy link

This PR introduces a TagValueRegexCollector that allows layers to be defined based on the presence or value of doc tags like @deprecated.

All tags present in the doc block are exposed on ClassLikeReference and FunctionReference, for use by collectors and event handlers.

Note that this changs the way #1319 and #1318 should be implemented.

@patrickkusebauch
Copy link
Collaborator

Looks really good. I will give it a thorough review later today. One thing I noticed is your usage of string[] in PDPdoc block for the tags. Did you mean there an array<array-key, string> or list<string>. It is not clear to me from the code there.

@brightbyte

This comment was marked as resolved.

Copy link
Collaborator

@patrickkusebauch patrickkusebauch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR should make your other contributions much easier. I would suggest getting this one closed first.

src/Contract/Ast/TaggedTokenReferenceInterface.php Outdated Show resolved Hide resolved
src/Contract/Ast/TaggedTokenReferenceInterface.php Outdated Show resolved Hide resolved
src/Core/Ast/AstMap/TaggedReferenceTrait.php Outdated Show resolved Hide resolved
@patrickkusebauch
Copy link
Collaborator

One thing I noticed is your usage of string[] in PDPdoc block for the tags. Did you mean there an array<array-key, string> or list<string>. It is not clear to me from the code there.

I think of string[] as the same as list<string>, though I guess it's not explicit in https://docs.phpdoc.org/guide/guides/types.html#arrays. Anyway, the same tag can be specified multiple times, so for a given tag name, there is a list of tag values.

list<string> is stricter and static analysis tools like PHPStan and Psalm can do more precise analysis in you specify it as strictly as possible. string[] is really an array<array-key, string>, where array-key is int|string while list<string> is a subtype of array<int, string>.

@brightbyte

This comment was marked as resolved.

@patrickkusebauch
Copy link
Collaborator

I'm confused by the psalm error. I am not seeign that locally. And when I remove the respective entries from the baseline file, I get psalm errors...

Change in CollectorConfig docstring is the cause of the issue, look at my other comment.

@brightbyte
Copy link
Author

This PR should make your other contributions much easier. I would suggest getting this one closed first.

Yes, agreed.

Copy link
Collaborator

@patrickkusebauch patrickkusebauch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One tiny docs change and LGTM.

src/Core/Ast/AstMap/TaggedTokenReference.php Outdated Show resolved Hide resolved
Copy link
Collaborator

@patrickkusebauch patrickkusebauch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🚀

@patrickkusebauch
Copy link
Collaborator

@gennadigennadigennadi when you have a minute, can you merge this?

@gennadigennadigennadi
Copy link
Member

I will response as soon as possible. Maybe during the weekend or the days after.

I was reading without interfering all your conversations @brightbyte and @patrickkusebauch. And thank you both for the work you put into this PR.

@brightbyte
Copy link
Author

I will response as soon as possible. Maybe during the weekend or the days after.

No rush, and happy holidays!

@gennadigennadigennadi
Copy link
Member

Just a thought I had, should we also support Attributes for this purpose in the future?

@brightbyte
Copy link
Author

Just a thought I had, should we also support Attributes for this purpose in the future?

Isn't that what AttributeCollector does?

@gennadigennadigennadi
Copy link
Member

Just a thought I had, should we also support Attributes for this purpose in the future?

Isn't that what AttributeCollector does?

Yup, it is. Have not seen it.

- renamed getParser to createParser
- make TagValueRegexCollectorTest final
- added test case names to data providers in DependsOnInternalTokenTest
@brightbyte
Copy link
Author

I'm confused by that CI error. Psalm is green when I run it locally.

@gennadigennadigennadi
Copy link
Member

The psalm errors have been introduced with the last composer dependencies update.
They need to be fixed soon.

@brightbyte have install the updated dependencies from main?

@brightbyte
Copy link
Author

@brightbyte have install the updated dependencies from main?

Ah, right. But also, it seems like I have messed up the rebase - I keep thinking on commits (gerrit flow) instead of branches (github flow). I'll look into it tomorrow.

@brightbyte
Copy link
Author

@gennadigennadigennadi Did I see correctly that this already went in as part of #1318, so this PR is redundant now? The fact that I don't make stacked PRs from a fork is really confusing me, it looks like #1318 ended up as two PRs mashed together...

@gennadigennadigennadi
Copy link
Member

@gennadigennadigennadi Did I see correctly that this already went in as part of #1318, so this PR is redundant now? The fact that I don't make stacked PRs from a fork is really confusing me, it looks like #1318 ended up as two PRs mashed together...

Looks like it did.

@gennadigennadigennadi gennadigennadigennadi merged commit 172da95 into qossmic:main Jan 22, 2024
17 of 18 checks passed
@gennadigennadigennadi
Copy link
Member

Nevertheless, also merged this PR. @brightbyte thank you, again.

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