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

Make writing relation count properties optional #837

Closed
wants to merge 3 commits into from
Closed

Make writing relation count properties optional #837

wants to merge 3 commits into from

Conversation

AegirLeet
Copy link
Contributor

#783 introduced writing of DocBlock comments for $relation_count properties, populated by withCount/loadCount. This makes writing those comments optional and configurable (still on by default).

@spawnia
Copy link
Contributor

spawnia commented Oct 17, 2019

I am interested in this configuration too.

Same as write_model_magic_where, i think this should be optional.

Copy link
Contributor

@spawnia spawnia left a comment

Choose a reason for hiding this comment

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

Nice job. A clean and simple improvement!

| If set to true, every "x to many" relation will generate an accompanying
| "$relation_count" property DocBlock comment.
|
| If set to false, no such DocBlock comments will be generated.
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems a bit redundant, i think the description is long and clear enough without this line.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I second this, please remove it

@AegirLeet
Copy link
Contributor Author

@barryvdh Any chance to get this merged?

@AegirLeet
Copy link
Contributor Author

Is this repo still maintained?

Copy link
Collaborator

@mfn mfn left a comment

Choose a reason for hiding this comment

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

I think this would be useful addition 👍

Even better if you could add a test!

| If set to true, every "x to many" relation will generate an accompanying
| "$relation_count" property DocBlock comment.
|
| If set to false, no such DocBlock comments will be generated.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I second this, please remove it

@PaulRotter
Copy link

I think this would be useful addition 👍

Even better if you could add a test!

Am I right in assuming that the missing test is the only thing, that prevents this PR from being merged?

@mfn
Copy link
Collaborator

mfn commented Jun 8, 2020

@PaulRotter I'm not the ultimate decision make here, but it's my believe that changes/added features should be accompanied with tests showing they work as intended.

I therefore made sure that, especially the ModelsCommand, can be tested and added an initial batch of tests so others can follow more easily; you should find enough examples to make a test and it shouldn't take you longer than the the actual implementation.

Personally I don't see anything (else) in the way of merging this.

@AegirLeet AegirLeet closed this Jun 23, 2020
@AegirLeet AegirLeet deleted the optional-relation-count-properties branch June 23, 2020 13:11
@AegirLeet AegirLeet restored the optional-relation-count-properties branch June 23, 2020 13:11
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