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

Adding columns defined in actAs-templates to the docblock of the generated model class #110

Conversation

thirsch
Copy link
Collaborator

@thirsch thirsch commented Jan 22, 2024

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.

@alquerci
Copy link

alquerci commented Jan 22, 2024

wdyt about this addition?

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.
Even a failing test, will be enough to sense it.

@connorhu
Copy link
Collaborator

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.

thirsch added a commit to vemaeg/doctrine1 that referenced this pull request Jan 23, 2024
@thirsch thirsch force-pushed the feature/adding-actas-columns-to-the-docblock branch 2 times, most recently from 69fefea to a2ed8ed Compare January 23, 2024 21:42
@thirsch
Copy link
Collaborator Author

thirsch commented Jan 23, 2024

I've updated the pr:

  • included a test case
  • don't throw an exception anymore, if the class defined as actAs does not exist
  • don't introduce a BC change as all new parameters are additional optional arguments to the public methods

wdyt?

@thirsch thirsch marked this pull request as ready for review January 23, 2024 21:43
@alquerci
Copy link

@thirsch it is better than before.

Let's add this new behaviour and make it with a good job.
It's a good opportunity to sense the effort to make changes there without fear.

How do you feel when making changes in this class?

@thirsch thirsch force-pushed the feature/adding-actas-columns-to-the-docblock branch from a2ed8ed to b6356a9 Compare January 24, 2024 07:42
@thirsch
Copy link
Collaborator Author

thirsch commented Jan 24, 2024

How do you feel when making changes in this class?

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.

@thirsch thirsch force-pushed the feature/adding-actas-columns-to-the-docblock branch 2 times, most recently from 06817da to e8aac05 Compare January 24, 2024 12:47
@alquerci
Copy link

this saves a lot of manual added typehints in the codebase

What does forces you to add these type hints ?

I have 2 hypotheses of usage and constraints, but I wish to know yours.

@thirsch thirsch force-pushed the feature/adding-actas-columns-to-the-docblock branch from e8aac05 to 7d67309 Compare January 24, 2024 12:48
@thirsch thirsch force-pushed the feature/adding-actas-columns-to-the-docblock branch from 7d67309 to 95da927 Compare January 24, 2024 13:21
@thirsch
Copy link
Collaborator Author

thirsch commented Jan 24, 2024

this saves a lot of manual added typehints in the codebase

What does forces you to add these type hints ?

I have 2 hypotheses of usage and constraints, but I wish to know yours.

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.

@thirsch thirsch force-pushed the feature/adding-actas-columns-to-the-docblock branch from 95da927 to 3de830a Compare January 24, 2024 19:31
@thirsch thirsch requested review from alquerci, connorhu and thePanz and removed request for alquerci January 24, 2024 19:33
@thirsch thirsch force-pushed the feature/adding-actas-columns-to-the-docblock branch from 3de830a to cb28ccb Compare January 24, 2024 20:03
@thirsch thirsch marked this pull request as draft January 24, 2024 21:27
@thirsch thirsch force-pushed the feature/adding-actas-columns-to-the-docblock branch from cb28ccb to 9b6de4a Compare January 24, 2024 21:37
@thirsch thirsch marked this pull request as ready for review January 24, 2024 21:38
lib/Doctrine/Import/Builder.php Outdated Show resolved Hide resolved
lib/Doctrine/Import/Builder.php Outdated Show resolved Hide resolved
lib/Doctrine/Import/Builder.php Outdated Show resolved Hide resolved
@thirsch thirsch force-pushed the feature/adding-actas-columns-to-the-docblock branch 2 times, most recently from b233339 to 3e49b73 Compare January 25, 2024 08:18
@thirsch
Copy link
Collaborator Author

thirsch commented Jan 25, 2024

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.

@thirsch thirsch force-pushed the feature/adding-actas-columns-to-the-docblock branch from 7908c77 to f6bf135 Compare January 30, 2024 19:04
@thirsch thirsch force-pushed the feature/adding-actas-columns-to-the-docblock branch 2 times, most recently from fca34fd to b78fdd3 Compare February 2, 2024 21:48
@thirsch thirsch force-pushed the feature/adding-actas-columns-to-the-docblock branch 2 times, most recently from 505b409 to a3e2a22 Compare February 3, 2024 19:42
Copy link
Member

@thePanz thePanz left a 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!

lib/Doctrine/Import/Builder.php Outdated Show resolved Hide resolved
lib/Doctrine/Import/Builder.php Outdated Show resolved Hide resolved
@thirsch thirsch force-pushed the feature/adding-actas-columns-to-the-docblock branch from a3e2a22 to 4bf5a41 Compare February 4, 2024 18:35
@thePanz thePanz merged commit eeb1777 into FriendsOfSymfony1:master Feb 5, 2024
5 checks passed
@thirsch
Copy link
Collaborator Author

thirsch commented Feb 5, 2024

Thanks for merging it :)

@thirsch thirsch deleted the feature/adding-actas-columns-to-the-docblock branch February 5, 2024 11:12
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.

4 participants