-
Notifications
You must be signed in to change notification settings - Fork 16
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
added feature for validating arrays regardless of the order of the eleme... #1
Conversation
Thanks for the PR, @odobroiu despite the commit message :) I only remember that this topic has been discussed, and that we decided for KISS - test only types and values - but not the for&against details. Do you @isakb? Personally, I have to reflect on this change and the alternatives. I am not so fond of the PR as it is since this adds array-specific semantics that are not related to the values themselves, but is a flag for the comparison algorithm. IFF more semantics are needed, then I'd prefer a bit more general approach. From the top of my had, create a new syntax like
and basically have a callback Ping @dmitriid - I guess this hasn't come to your attention begging for a fix? EDITED: unordered_array is a set |
I think the main argument was simplicity, but if there were any other reasons I have forgotten. I also am not a huge fan of the syntax here, and I think we were discussing something similar and came to the conclusion that it was not pretty enough to keep and we cursed JSON/JavaScript for not having sets. |
@isakb thoughts on the extension proposal |
I don't really like that either, and am thinking about a totally different approach. But I guess if there is an immediate need for the functionality it would be acceptable. |
Different approach?
|
@andreineculau, sorry for the PR message... I thought the commit message and the readme was enough... but yes, I agree it would have been nice to write a few words there as well :) About the change, I did it that way because thought it would be similar to how the {{unexpected}} tag works, in the sense that it also implies that the comparison is done either by exactly matching the 2 arrays, or by testing that the expected array is included in the actual array. Either way, I do not have strong opinions about the sintax, but I do think that this feature should be supported( one way or the other). About the possible options, I guess all the options so far are: As is in PR
Andrei's proposal:
Similar, but maybe simpler
Not sure how I feel about this one, but added it here
Also, maybe the set is not quite a good word, since that would imply elements are unique. What do you think? |
good Monday morning input, @odobroiu :) thanks i sort of like the last proposal, but i need to sleep on it On Mon, Nov 3, 2014 at 10:28 AM, odobroiu notifications@github.com wrote:
andreineculau.com http://www.andreineculau.com |
Realized that there is some more work to do for this, as when sorting the array the initial order is lost and therefore the error message might be confusing. Will address this after we agree on the syntax. |
pinging @sstrigler to take a stand On Tue, Nov 4, 2014 at 9:34 AM, odobroiu notifications@github.com wrote:
andreineculau.com http://www.andreineculau.com |
Sorry, somehow I wasn't in the loop here. As for the approach {
"list_of_objects": {
"{{compare}}": "as_set", ~~"unordered_array"~~
"items": [3,2,1]
}
} I wonder about alternatives for {{compare}}, don't we compare always? Then it sounds a bit redundant. What if we'd allow are more json-schema like approach (in addition to what we already have)? {
"list_of_objects": {
"type": "set",
"value": [1,2,3]
}
} |
Ok, that was stupid. Next try: {
"list_of_objects": {
"{{type}}": "set",
"value": [1,2,3]
}
} which is pretty close to what Andrei proposed. |
I was thinking that we need a JSON parser with some extensions. Then the syntax could be more readable. Something like:
(Unicode arrows = Github flavored markdown workaround) |
@isakb I don't know if the others have given any thought so far, I did. Similar to Apiary, love at first sight and ease of use happens because of the familiar syntax - HTTP meta + payload (JSON). Then come the assertions/extensions. Apiary's new syntax, markdown based, and your suggestion is 180 degrees from their first grammar (which KATT is based on). The same way I was vocal about Apiary's change, I vote against this radical change. But above all, I see in my proposal the chance for more dynamic and readable extensions e.g.
But it would be good with more input :) @sstrigler @dmitriid ? Short-term, it would be good to see a PR with say @sstrigler 's last comment, that implements support for comparison extensions, implements a "set" comparison extension, and a few tests. Do you have time @odobroiu ? If not, I'll put it as a Xmas todo for myself :) |
@andreineculau I'm not so sure if you'll have time around Xmas as I'll be in town 💃 |
As requested my 2c. I also think that @isakb's approach is too far away from the initial idea of using that version of apiary's blueprint file format. At least I can't see how this relates to what we have now. Another thing that comes to my mind: What if we'd build in support for validating schemas (as in json-schemas for now) which then could contain those constraints?
|
Maybe forget my last comment, it's derailing the discussion. |
The proposed syntax additions are far from familiar to my mind, and seeing that only some of the special keys have the double bracket "escaping" it also violates the principle of least surprise. And since we have already broken away from APiary blueprint compatibility (or they have...) there is little reason to try to keep the original syntax unless it has some inherent benefit. The original goal of keeping it simple was already a lost cause due to the constraint that we needed to have the KATT symbols be a part of the expected response, instead of being able to escape from it in a good way. That makes for a terse syntax with complicated workarounds that are difficult to remember. That is the reason why I was thinking of taking a different approach, but like I said if you have a need for the new syntax then I really don't mind, as I am not actually using KATT at all these days, so this change will not affect me. Sent from my iPad
|
Created a new PR about this. Should probably close this one. |
Closing on the basis of
|
...nts