-
Notifications
You must be signed in to change notification settings - Fork 75
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
Adding columns defined in actAs-templates to the docblock of the generated model class #110
Adding columns defined in actAs-templates to the docblock of the generated model class #110
Conversation
It is an interesting idea, to provide better developer experience. However, I am not sure about the time to invest on this new feature. In any case, I like to read tests as they show examples. |
I am a progressive person. If the change will move your work forward or you see it as helping others in their work then go for it. |
69fefea
to
a2ed8ed
Compare
I've updated the pr:
wdyt? |
@thirsch it is better than before. Let's add this new behaviour and make it with a good job. How do you feel when making changes in this class? |
a2ed8ed
to
b6356a9
Compare
Well, I still think we should keep changes in the sense of new features to a bare minimum or try to avoid it at all. For this particular topic, it was more like a bug to us, as this saves a lot of manual added typehints in the codebase. Sure, it is no bug as such, as it was absolutely not a topic back in the days when this code base was written. |
06817da
to
e8aac05
Compare
What does forces you to add these type hints ? I have 2 hypotheses of usage and constraints, but I wish to know yours. |
e8aac05
to
7d67309
Compare
7d67309
to
95da927
Compare
It's primarly a question of inconsistency of having the "direct" fields documented whilst the "inherited" once are not. Points are: ide support (code completion, type checks), tools like psalm/phpstan and even good old pull request review, where the fields are there and can be found by checking what has been created/changed. |
95da927
to
3de830a
Compare
3de830a
to
cb28ccb
Compare
cb28ccb
to
9b6de4a
Compare
b233339
to
3e49b73
Compare
I've changed the loosely testing "contains"-checks to a snapshot testing to avoid sideeffects being generated. IMHO this even allows a clearer view on what exactly is this change doing. |
7908c77
to
f6bf135
Compare
fca34fd
to
b78fdd3
Compare
505b409
to
a3e2a22
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some minor comments, LGTM otherwise!
Thank you for your work and commitment! Kudos!
…rated model class.
a3e2a22
to
4bf5a41
Compare
Thanks for merging it :) |
The generated docblock does not contain the columns defined in the referenced behaviours (actAs). This patch adds those columns to the docblock.
@alquerci, @connorhu, @thePanz, wdyt about this addition? I'd create a test for it, if you are interested in the change at all.