sangria icon indicating copy to clipboard operation
sangria copied to clipboard

Do not force QueryValidator to use List

Open bjing opened this issue 3 years ago • 3 comments

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.

bjing avatar Oct 15 '21 05:10 bjing

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?

yanns avatar Sep 07 '23 16:09 yanns

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.

bjing avatar Sep 18 '23 01:09 bjing

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.

yanns avatar Sep 18 '23 06:09 yanns