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

Do not force QueryValidator to use List #751

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

bjing
Copy link

@bjing bjing commented Oct 15, 2021

Do not force the use of List type for rules when creating a QueryValidator.

List is not a very efficient type and forcing the clients to create rules in List has no obvious benefit because Sangeria is not using any functionality from an actual List type.

This gives the users the flexibility to use the types they need, if they need List or Vector or anything else, this will allow them to do it.

Also, for the def validateUsingRules() function, since visitors parameter only gets iterated once, there's no need to mandate any collection type other than an Iterable.

@yanns
Copy link
Contributor

yanns commented Sep 7, 2023

Sorry, I somehow missed that PR.
Do you have examples when this optimization is relevant?

The list of rules is normally created once with the schema for the whole application's life. Given that, I am not sure if this optimization brings much value.
But maybe you're using the library another way?

@bjing
Copy link
Author

bjing commented Sep 18, 2023

Hi @yanns thanks for getting back to me. I no longer work at the place where this change is needed, and I can no longer provide an example where this causes an issue, it was indeed an issue for us back then.

However, from a design point of view, I think it's always nice to use more of a base type (or interface) instead of something like a List, especially when you're not using any features from a List. This would give the users more flexibility in terms of what they can do.

Anyway, since I no longer work with Sangria, feel free to disregard this PR.

@yanns
Copy link
Contributor

yanns commented Sep 18, 2023

I see your point.
But this change is not backwards compatible. I need to check if the reasons are good enough.
Let me think about it.

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.

None yet

2 participants