Conversation
vishakha041
left a comment
There was a problem hiding this comment.
The API change is important to fix. Rest looks good.
utils/src/api_schema/api_schema.json
Outdated
| "sum": { "type": "string" }, | ||
| "limit": { "$ref": "#/definitions/positiveInt" }, | ||
| "sort": { "type": "string" }, | ||
| "descending": { "type": "boolean" }, |
There was a problem hiding this comment.
Is there no way to make this part of the sort command? Descend keyword by itself doesn't make sense. It is useful only in the sort context.
There was a problem hiding this comment.
yep, I was divided about this as well. Adding a "sort block" that has a prop for sorting and the option of using order = "ascending" or order = "descending", but this will break backward compatibility of all the old queries that uses sort.
There was a problem hiding this comment.
Will it still be possible to have a default so you can only specify the key ?
There was a problem hiding this comment.
Ok.
The Options are:
-
(breaks backward compatibility)
"sort": {
"key": "property_used_for_sorting",
"order": "ascending" // or "descending", with the default to one of them.
} -
As it is implemented, just adds the order as an extra element of the results block, that will only be used if "sort" is also specified.
-
We can change "descending" = [True, False] to "order" = ["ascending", "descending"]. I like this option more, which does not break compatibility, it just adds more flexibility.
There was a problem hiding this comment.
Is there no way to support either a value or a block, as:
"sort" : "<key>"
and
"sort" : { "key" : "<key>", "order" : "descending" }
If not, I'm inclined to go with the backward compatible option, with the addition
"sort-order" = ["ascending","descending"]
tests/python/TestEntities.py
Outdated
| self.assertEqual(response[0]["AddEntity"]["status"], 0) | ||
| self.assertEqual(response[1]["AddEntity"]["status"], 0) | ||
|
|
||
| def test_FindAscending(self): |
There was a problem hiding this comment.
Shouldn't this be called FindDescending? Or FindWithSortOrder :)
utils/src/api_schema/api_schema.json
Outdated
| "sum": { "type": "string" }, | ||
| "limit": { "$ref": "#/definitions/positiveInt" }, | ||
| "sort": { "type": "string" }, | ||
| "descending": { "type": "boolean" }, |
There was a problem hiding this comment.
Is there no way to support either a value or a block, as:
"sort" : "<key>"
and
"sort" : { "key" : "<key>", "order" : "descending" }
If not, I'm inclined to go with the backward compatible option, with the addition
"sort-order" = ["ascending","descending"]
|
I like what you propose @philiplantz. For some reason I have a memory that we cannot do that. |
|
I like the option Philip has mentioned. Otherwise option 1 from Luis. It will break backward compatibility but will open the way for allowing sort on multiple keys and so on in the near future. |
115e3fe to
1a36838
Compare
|
Sorting multiple keys is independent for of the having a sort block or not. I will go with option 2 to avoid braking compatibility for now, and change the keywords following Philip suggestion which is much nicer. |
|
Good news, after trying many different schema configurations, it is indeed possible to have: Now I wonder what was the memory both @philiplantz and I have, and if we can make some other checks better. But at least now we are sure we can. |
|
For multiple keys we will need a sort block but never mind for now. Its good that the other option worked. Cool. |
|
When can we have this in the master branch? Or even released? |
|
Our "develop" branch is like a master branch. We only commit to master with releases. |
Solve #52