feat: Unify CRUD in dialog diff #2
Conversation
Pull Request Test Coverage Report for Build 85640374
💛 - Coveralls |
| IStopper, | ||
| defualtJSONStopComparison, | ||
| getWithJsonPath, | ||
| } from './jsonDiff'; |
There was a problem hiding this comment.
I think you used a lot of interfaces, showing you trying to think this as a more general level, which is a good design thinking.
But there are two things that are as important as a generic mindset
- if you are thinking from a generic perspective, make sure the concepts in this layer are consistent, which means, make sure they are all high-level concept or low-level concepts. Check your abstractions, are they mixing high-level and low level concept, are they mixing interface and implementation, are we have very similar things.
- Both less engineering and over engineering is not good, but usually over engineering hurts more than less engineering. This is usually a result of 1.
| IJSONChangeDelete, | ||
| IJSONChangeUpdate, | ||
| IComparator, | ||
| IStopper, |
There was a problem hiding this comment.
In this case, Comparator is a generic thing, which make sense, but Stopper feels weird, stopper is just a sepecifc process in your walker, if i understand correctly.
| preValue: any; | ||
| } | ||
|
|
||
| export interface IJsonChanges { |
There was a problem hiding this comment.
This is not a good interface, usually you don't see changes as interface name, because you can use built-in array or dict to capture the abstraction of a container.
I think if you want to make it generic,
you would expect a Differ will diff two json and returns a set of changes.
interface JsonDiffer {
differ(jsonA, jsonB) :=> JsonDiff[]
}
in this definiton, there is only minimal concept that, a json differ diff json into diffs. that's it, as high level.
at low level, is you may have different type of Diff, that's totally a low level differend kind of type of JsonDiff.
all those above is just showing what mean by high-level vs low-level abstraction. But in our case, we probabaly don't need any of them.
| updates: IJSONChangeUpdate[]; | ||
| } | ||
|
|
||
| export type IComparator = (json1: any, json2: any, path: string) => { isChange: boolean; isStop: boolean }; |
There was a problem hiding this comment.
In this comparator interface, too much concept here, there is a path, there is IsChange and isStop and no result returned, which is weird.
what a comparator should represent? in almost any programming language, it usually takes two object of same type, and result in a true\false comparasion result.
| if (path === JsonPathStart) return true; | ||
| const objPath = jsonPathToObjectPath(path); | ||
| return has(value, objPath); | ||
| } |
There was a problem hiding this comment.
Those 3 jPath related functions, what's the difference with standard jpath library provided and we must provide at here?
| * @param list2 | ||
| * @param comparator // value comparator | ||
| */ | ||
| export function ListDiff(list1: any[], list2: any[], comparator?: IComparator): IJsonChanges { |
There was a problem hiding this comment.
This is a similar conceptual question, what the top-level abstraction.
If i as a user, see a JsonDiff, and ListDiff, it's confusing, because list is a valid json. So JsonDiff is a super set of listDiff, listDiffer maybe make sense, but it's a "implemantation details" of how you achieve JsonDiff.
Description
By unify lg/lu CRUD in dialog, could significantly decrease knowledges
extensionsneed aware, only focus on *.dialog file, let shell handle rest related resource like *.lg *.lu file.Workable checklist
Visual Editor
Form Editor
Shell - Toolbar
Shell - ProjectTree
Resource types X Operations
Task Item
refs microsoft#2699
refs microsoft#2699
Screenshots