Conversation
|
Still a work in progress, however, its at the stage to finalize changes/review |
umesh-timalsina
force-pushed
the
46-formalizing-patch
branch
from
aca9948 to
d3b4d4d
Compare
brollb
left a comment
brollb
left a comment
There was a problem hiding this comment.
Nice. I added a comment about how we might be able to refactor it a little further to remove some of the remaining boilerplate!
| ); | ||
| } | ||
|
|
||
| validated(fn: (ch: NodeChangeSet) => boolean, errMsg: string): Result<NodeChangeSet, Error> { |
There was a problem hiding this comment.
If we are passing the validation function here (which is only called once per patch operation right before applying it), it probably doesn't really buy us much defining this here. I know I initially suggested something like this but it might make more sense to just define a validation function for patch operations. I made an example of what I mean in a commit and pushed it.
| this.nodeSearchUtils = nodeSearchUtils; | ||
| } | ||
|
|
||
| keyLengthValidator(change) { |
There was a problem hiding this comment.
Neither would this (at least not on the base class where I am not convinced it makes the most sense).
There was a problem hiding this comment.
Kept it for reuse in couple of places
| } | ||
|
|
||
| async put(node: Core.Node, change: NodeChangeSet, resolvedSelectors: NodeSelections): PatchResultPromise { | ||
| return this._validChange(change) |
There was a problem hiding this comment.
This is an example of what I meant. Each patch operation just defines their own _put and _delete methods which accept a validated change. Validation can be defined in another method, _validChange. (I am sure there is a better name - maybe getValidChange or something?)
There was a problem hiding this comment.
There are two changes I had to make:
- There should be two validators:
validChangePutandvalidChangeDelete. - Not every error can be resolved (deletion for MemberAttributes for example with the above signature).
| _put = async (node: Core.Node, change: NodeChangeSet, resolvedSelectors: NodeSelections): PatchResultPromise => { | ||
| const [/*type*/, name] = change.key; | ||
| this.core.setAttribute(node, name, change.value || ''); | ||
| return new PatchResult(change); // I am not sure we need this PatchResult as it seems that it only ever returns the NodeChangeSet that was passed to it...? |
There was a problem hiding this comment.
I didn't remove PatchResult but does this ever return something other than the change that was applied?
There was a problem hiding this comment.
Based on your suggestion, I have created a PatchInfo object and the signature of _put and _delete methods go as follows:
class PatchInfo {
nodeId: GmeCommon.Path;
nodeGuid: Core.GUID;
target: string;
appliedPatch: ChangeSet;
}
type PatchFunction = (node: Core.Node, change: NodeChangeSet, resolvedSelectors: NodeSelections) => Promise<void>;
2 participants
No description provided.