-
Notifications
You must be signed in to change notification settings - Fork 7
Rewrite/overloadmap as tree #101
Rewrite/overloadmap as tree #101
Conversation
Pull Request Test Coverage Report for Build 1116703798
💛 - Coveralls |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added some comments, but I don't fully understand what the code does; likely due to the minimal documentation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks much clearer now :-)
Let's wait for @wschella's feedback, as this change is pretty invasive.
lib/functions/Helpers.ts
Outdated
return this.add(new Impl({ types, func })); | ||
} | ||
|
||
public copy({ from, to }: { from: ArgumentType[]; to: ArgumentType[] }): Builder { | ||
const last = this.implementations.length - 1; | ||
const _from = List(from); | ||
// TODO: Does this need to be a copy? - ask in PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll talk about both TODOs here.
What this function does is copy the implementation from one overload (i.e. a list of types) to another. So basically it makes two overloads behave identical. What this loop (and the equals) does is iterate over the list of existing (types, implementation) pairs (i.e. overloads) and when it finds a match with _from
, it copies that implementation and it's a new overload with this implementation and the types as defined in the to
argument.
So the arraysEqual is necessary if we want to keep logic like this. This is okay as it's done only once. This Builder class is simply the 'DSL' I talked about: it's a way to create overloaded function definitions for our SPARQL functions with the Builder pattern. It's all static info.
Now, you don't necessarily need to keep this logic. If instead of keeping a a list of implementations (this.implementations
), you just build the OverloadNode directly, you could maybe use the search
method here, although it would have to a searchExact
, we only want exact type matches for this function. This will clean things up, but it would require rewriting most of the Builder functions (e.g. onUnary
) to operate on the tree instead of the list. This should not be too difficult.
The copy (the first todo) is not necessary, with the same reasons as the one in the .set
function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll leave rewriting this file for another time. I removed the TODO's :) Thank you!
This pull request aims to provide a first implementation for transforming the overloadmap to a tree structure. This removes it dependency on the 'immutable' package and should allow us to auto cast some concrete types.
In the first version of this PR I added 3 TODO's. These should not be merged into master but should be discussed here and resolved before merging.