feat: PhpdocTypesFixer - support multiline array shapes#8893
Conversation
931421a to
b30b6fc
Compare
| (?<array_shape> | ||
| (?<array_shape_name>(?i)(?:array|list|object)(?-i)) | ||
| (?<array_shape_start>\h*\{\h*) | ||
| (?<array_shape_start>\h*\{[\h\v*]*) |
There was a problem hiding this comment.
I think the main regex for types should not support *. * is not part of phpdoc grammar.
If this approach is needed, keep the original regex as is and let's have a 2nd one with something like str_replace('\h', '[\h\v*]', ....
My preference would be to never mix this parsing together and use proper nesting:
- parse
/**comments - strip
/**,*at following lines and*/ - now fix the "parsed comment content"
- restore
/**, ˙*˙...
There was a problem hiding this comment.
I think the main regex for types should not support
*.*is not part of phpdoc grammar.
How * is not part of phpdoc grammar?
There was a problem hiding this comment.
I mean part of the phpdoc type. IMO you simply mix the layers here. One for phpdoc comment, one for phpdoc type.
There was a problem hiding this comment.
Multiline PHPDoc types are, in fact, types "polluted" with whitespaces and *.
I wanted the simplest - in terms of changes - solution, feel free to propose an alternative with str_replace('\h', '[\h\v*]', ...).
There was a problem hiding this comment.
I mean to have a static method returning the regex with the solution you propose now and cached. This way I would be happy as the regex will remain untouched and someone else can impl. the idea from #8893 (comment) later.
To support my domain/layer claim, imagine the regex used in Markdown or // comments, in both cases ˙*˙ is wrong/not part of the type grammar.
There was a problem hiding this comment.
I am the author of the complex type regex and I say this is wrong, we would have context-dependent regex which is really not good. It is hack. I know I am not an official maintainer, I hope can I express change request for this.
There was a problem hiding this comment.
I say this is wrong
And I say it is right, which is as much as my opinion as yours is your opinion.
You say a lot of "should", "must", etc. - I like to stick to the facts, and those are:
- no other proposition in over 3 years
- new feature is supported
- nothing gets broken
There was a problem hiding this comment.
for sake of solving issue from 2021, i'm OK to merge this as-is.
if you have even better proposal how to fix this @mvorisek , please raise follow-up PR, unless it would make things over-complicated. I think analysing "real content" without "*" could be more pure, but i'm afraid it would overcomplicate code without any benefit. happy to see the PR showing it's easy and elegant.
There was a problem hiding this comment.
I understand you guys but I simply view things differently. I work mostly on databases where integrity is important and no compromises are allowed. This tool is a piece of art and I probably should hand over my work on this topic to the next hands. Enjoy the summer! 🏝️
There was a problem hiding this comment.
i see value in your original idea, we simply need to get pragmatism when it's possible, otherwise the issue will stay open another 4 years. "Perfect is the enemy of good" and "we released in perfect way, just 5 years delayed"
Closes #6078