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 #969

Merged
merged 4 commits into from
Jun 23, 2020
Merged

Make writing relation count properties optional #969

merged 4 commits into from
Jun 23, 2020

Conversation

AegirLeet
Copy link
Contributor

Accidentally fucked up #837 by deleting the branch or force-pushing (can't reopen), so here's a new PR.

I've rebased on top of current master, added a test for write_model_relation_count_properties and also added another test for write_model_magic_where while I was at it.

CC @mfn

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.

Implementation looks fine, but the way I understood it, you added two tests:

  1. one showing the default behavior, supposedly generating_count properties
  2. one showing it can be disabled

Maybe I missed it, but it seems like only 2) works as intended as 1) (…\MagicWhere\…) doesn't show any _count 🤔

@AegirLeet
Copy link
Contributor Author

The ModelsCommand/MagicWhere part isn't related to _count properties, I just noticed it was missing a test and decided to add it as well.

_count being added (default behavior) is already tested in some other tests, like in ModelsCommand/Relations. So ModelsCommand/RelationCountProperties only tests that they disappear when the new option is set to true.

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.

Purrfect 🐱 :-)

@barryvdh barryvdh merged commit cca99b6 into barryvdh:master Jun 23, 2020
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.

3 participants