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

Support skipping fields in Queryable derive (falls back to Default impl) #2933

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Ten0
Copy link
Member

@Ten0 Ten0 commented Oct 27, 2021

Not sure if there was a feature request for this, but I happened to need it and felt it wouldn't be too hard to implement.

Does this seem reasonable? (We perhaps just want this also be available on the other derives?)

@Ten0 Ten0 self-assigned this Oct 27, 2021
@weiznich
Copy link
Member

See #1582 and #860 for prior discussion.
That written: I'm worried about such a feature as it opens to door for users to easily construct N + 1 query constructs instead of using the associations API.
This means I would like to see a good motivation why this is necessary and why this cannot be solve differently.

@Ten0
Copy link
Member Author

Ten0 commented Oct 28, 2021

Oh I did not expect there to be so much discussion on that topic already!
Funny how we had started the same exact implementation, though I iterated a bit more.

Overall not super convinced by the arguments against there, but turning this into a draft as there seems to not be a consensus towards implementation.

@Ten0 Ten0 marked this pull request as draft October 28, 2021 12:44
@AJTJ
Copy link

AJTJ commented Jun 28, 2023

Would love if this were to be picked up again :)

@Ten0
Copy link
Member Author

Ten0 commented Jun 28, 2023

The problem here is not workforce but the decision on the relevant discussion. If you have new insights on the topic feel free to add them there. 🙂

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