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

Reserve 'distinct' query parameter #943

Closed
wants to merge 1 commit into from

Conversation

daurnimator
Copy link
Contributor

For #915

@ruslantalpa
Copy link
Contributor

i don't see the point in this PR.
it's not like it will warn users not to use this keyword, it will just silently drop it from consideration leaving users to wonder what's going on.

In addition to that, it was not decided that a reserved keyword is the way to go here,
distinct is not on the same level as select/where/from/order ... this is something that is a modifyer in the select part.

@daurnimator
Copy link
Contributor Author

distinct is not on the same level as select/where/from/order ... this is something that is a modifyer in the select part.

distinct is a sql reserved word in every sense see https://www.postgresql.org/docs/current/static/sql-keywords-appendix.html

@ruslantalpa
Copy link
Contributor

this is not what i was saying, distinct operates in the SELECT space, it does not stand on it's own like WHERE/GROUP/ORDER.

i.e. the place for this feature in in the &select=...distinct... space, and not a top level keyword

@daurnimator
Copy link
Contributor Author

daurnimator commented Aug 15, 2017

i.e. the place for this feature in in the &select=...distinct... space, and not a top level keyword

I disagree, as noted in #915 (comment) we need to be able to provide an optional value for distinct and as a matter of http api taste, I think this belongs at the top level.

@ruslantalpa
Copy link
Contributor

and what will you do when the next logical request will be "i need to request distinct entities on the embedded entities"? :)
and i don't see any problems with the value you are saying, it's even more natural
select=distinct(id),name,subitems(distinct(id),name)

@daurnimator
Copy link
Contributor Author

and i don't see any problems with the value you are saying, it's even more natural
select=distinct(id),name,subitems(distinct(id),name)

Sure; that works too. Though it does look quite a lot like a resource embedding.

@begriffs
Copy link
Member

begriffs commented Aug 20, 2017

I agree with Ruslan here that disregarding a parameter without raising a warning, and not acting on it in any other way, could really leave people scratching their heads.

Also the first item feels more cohesive to me than the second in this list:

  • ?select=distinct(id),name,subitems(distinct(id),name)
  • ?select=id,name,subitems(id,name)&distinct=id,subitems.id

Although I admit that this may be purely subjective. Perhaps we'll find that one way is easier to parse than the other and that might influence our choice (as daurnimator pointed out distinct(foo) looks a lot like subitem(foo))

In any case, I'm closing this pull request as-is because until we act on the distinct param I believe it's confusing to silently remove it.

@begriffs begriffs closed this Aug 20, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants