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

Columns for virtual attributes not selected #9

Merged
merged 4 commits into from
Aug 1, 2017

Conversation

mattleff
Copy link
Contributor

When a model has virtual attributes that depend on columns that aren't being selected the virtual attributes cause the resolver to return null. This can be "fixed" by adding return null here. Would it be possible to make _filterForSelects() optional?

@mattleff mattleff mentioned this pull request Jul 14, 2017
@koskimas
Copy link
Collaborator

Sorry, I have totally missed your pull requests. Thank you for them! I'll take a look at them as soon as I can. I'm on a vacation so it may take a day or five 😀

@mattleff
Copy link
Contributor Author

@koskimas (from here)

An option like selectFiltering sounds like a reasonable solution to this.

I just saw your edit now. I'll work on adding this to this pull.

@mattleff
Copy link
Contributor Author

From here:

I don't like the idea that users need to remember things like "if I use fragments, I need to call this arbitrary selectFiltering(false) method".

^ I totally agree. Is there a way that we could avoid a similar What?! for virtual attributes? Would it be better if the method was named like allowVirtualAttributes(true)? How can we communicate that there's a connection between selectFiltering and virtual attributes that reference columns? Wdyt about logging a warning if a model has virtual attributes and select filtering is not disabled?

@mattleff
Copy link
Contributor Author

mattleff commented Aug 1, 2017

@koskimas I think this is ready for review now.

@koskimas
Copy link
Collaborator

koskimas commented Aug 1, 2017

Awesome! Thank you so much! I'll release a new version with your additions as soon as I get to my computer.

@koskimas koskimas merged commit a7ed624 into Vincit:master Aug 1, 2017
@mattleff mattleff deleted the virtual-attributes-failing branch August 1, 2017 15:55
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.

2 participants