Conversation
|
I expect that I have missed some obvious things, and that the implementation is not efficient esp using |
|
@almarklein , could you approve the workflow for this please. |
| if (!x) { return s; }; | ||
| if (typeof x==="object" && !Array.isArray(x)) {x = Object.keys(x)} | ||
| for (var i=0; i<x.length; i++) { | ||
| if (!s.has(x[i]) && !FUNCTION_PREFIXop_contains(x[i], s)) { s.add(x[i]) }; |
There was a problem hiding this comment.
We can do just s.add here, because JS's set filters duplicates by itself.
| C = ast.Set | ||
| if (isinstance(node.left_node, C) and | ||
| isinstance(node.right_node, C)): | ||
| if node.op == node.OPS.BitOr: | ||
| return self.use_std_function('op_set_union', [left, right]) | ||
| if node.op == node.OPS.BitAnd: | ||
| return self.use_std_function('op_set_intersection', [left, right]) | ||
| if node.op == node.OPS.BitXor: | ||
| return self.use_std_function('op_set_sym_difference', [left, right]) | ||
| if node.op == node.OPS.Sub: | ||
| return self.use_std_function('op_set_difference', [left, right]) |
There was a problem hiding this comment.
I'm a bit unsure about this. It allows set operations on literal sets only. The use is rather limited because in most cases you'd just directly write the result. But more importantly, it may fool users into thinking that operators work for sets, which is not the case (except for literals).
There was a problem hiding this comment.
I'm happy to drop these for the moment. They are not in my actual use case. I just included them to outline how they could be implemented on top of the JS Set.
|
We can add |
|
Thanks for this! You seem to have found your way around the codebase well, and also added tests. Nice! This looks good. I added few comments. I think we should drop the operators for literals to avoid confusion, but we could add support for e.g. |
2 participants
Fixes #24