-
Notifications
You must be signed in to change notification settings - Fork 7
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
variableDiff misses documentation #37
Comments
It would be good indeed to have a file with some more tests and examples. Especially the different behaviors of the I'd say that the whole interface is still provisional, once it is safe, I'd like to refactor it into Steno, and at the same time make VarDiff more general so it can be moved to the default library. |
Hi Julian,
Just got back from Austria, so I have a number of things to attend to before I dive back in.
But let's talk about the interface and why VarDiff works differently from DiffString:
0. First a bit of philosophy: It seems to me that DiffString is written like a stream filter. I found that
everything got easier and more flexible by treating the diff operation as an array manipulation. In particular,
it simplifies reordering existing characters.
1. For example, in DiffString, the index "i" shared by all the change functions enforces a coupling between
the source and target strings. That prevents matches that alter the order of existing chars.
2. Also, when would removeFunc ever need to know the current length of the target string being constructed?
It really acts in relation to the source string. Treating the deletion and insertion positions as the same just makes things awkward.
3. Similarly, "swap" appears to be a deletion and insertion at an index that happens to be within the range of
the source string. This seems to reflect the implementation rather than the desired semantics.
4. So, I would propose that delete, insert, retain are the natural categories and swap should be eliminated.
RJK
On Nov 13, 2017, at 4:26 AM, Julian Rohrhuber <notifications@github.com<mailto:notifications@github.com>> wrote:
It would be good indeed to have a file with some more tests and examples. Especially the different behaviors of the VarDiff variants should be made explicit and also tested. @rkuivila<https://github.com/rkuivila> could you do that?
I'd say that the whole interface is still provisional, once it is safe, I'd like to refactor it into Steno, and at the same time make VarDiff more general so it can be moved to the default library.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub<#37 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/ACHUyF1uPIGkB22SYgKQArRIVFiVVd05ks5s2AtOgaJpZM4QbcM0>.
|
Well, there is not much philosophy behind
Yes, why not. I think it might be better to make these operations part of the external interface of steno, so you can also put a letter somewhere by hand. My suggestion is that I open a branch where I do a refactoring until |
Hi Julian,
OK, sounds good. I will generate some more examples for the current fork.
But, thinking about making VarDiff general purpose...
It strikes me that there is no "correct" order of execution of update functions.
Deletions might be need to be before or after additions depending on what is being represented.
So, how to engineer this is not really clear.
If we treat char, sourceIndex, and targetIndex as arguments, their values
actually identify which function to call:
targetIndex == Nil ==> removeFunc
sourceIndex == Nil ==> addFunc
target and source notNil ==> keepFunc
So the user could iterate that array of triples and select functions based on the args.
That would give the user the option to reorganize things as needed with a sort.
Wait a day, as I may take a quick crack on that overnight. If so, I will push it as an added class
(VarDiffString).
Cheers,
RJK
[formatted by telephon for readability]
|
Yes, that's true for the general case. But will indices stay consistent with order? When removing an item you'll have to keep track of the shift of indices that come after it.
Actually, thinking about it again, I'd say the opposite: reducing swap to remove + insert is exposing an operational implementation detail. Swapping one element against another has a specific meaning, that in some cases you might want to account for (e.g. one might want to swap a synth, but keep the parameters). But for the current case it doesn't matter and I don't mind if we don't want to support it. |
+1 |
changes introduced in #26 are lacking documentation.
The text was updated successfully, but these errors were encountered: