search

qossmic / deptrac

Keep your architecture clean.
https://qossmic.github.io/deptrac
MIT License
2.61k stars 135 forks source link

Define layers based on doc tags #1326

Closed brightbyte closed 7 months ago

brightbyte commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

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.

patrickkusebauch commented 9 months ago

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 commented 9 months ago

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...

patrickkusebauch commented 9 months ago

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 commented 9 months ago

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

Yes, agreed.

patrickkusebauch commented 9 months ago

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

gennadigennadigennadi commented 8 months ago

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 commented 8 months ago

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

No rush, and happy holidays!

gennadigennadigennadi commented 8 months ago

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

brightbyte commented 8 months ago

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

Isn't that what AttributeCollector does?

gennadigennadigennadi commented 8 months ago

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.

brightbyte commented 8 months ago

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

gennadigennadigennadi commented 8 months ago

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 commented 7 months ago

@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 commented 7 months ago

@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 commented 7 months ago

@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 commented 7 months ago

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