Refactoring sqPath #2986
Refactoring sqPath #2986
Conversation
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
2 Ignored Deployments
|
* main: VoidWidget for empty result values cleanup some types empty programs with possible comments are valid minor cleanup cleanups; remove some utility functions extract ViewerBody and ViewerMenu components
| return valuePathEdgeIsEqual(this.value, other.value); | ||
| } | ||
|
|
||
| toDisplayString(): string { |
There was a problem hiding this comment.
JS objects always have toString(), so someone could call it by accident and get [object Object].
I recommend renaming either toDisplayString() or uid() to toString().
There was a problem hiding this comment.
Good point.
How about I make a new toString() method that calls one of them (probably "toDisplayString". That way users could call toDisplayString, which I think is more precise.
| }: { | ||
| ast: ASTNode; | ||
| offset: number; | ||
| }): SqValuePath | undefined { | ||
| return new SqValuePath({ | ||
| root: "bindings", // not important, will probably be removed soon |
There was a problem hiding this comment.
Thoughts on this old comment:
- I don't believe anymore that we'll remove the root info here, it's important
viewValuePathinViewerProvideris currently broken when "Result" or "Imports" tab is selected, or even "Exports"; we should switch the dropdown to "Variables" when "Find in viewer" hotkey is pressed, and be more clever about "Exports" (maybe you already noticed this when you added gutter circles)- we could implement "find in editor" on imports, but we'd have to switch the viewer to "Imports" tab, and I'm not sure if it's worth it since we'll also get tooltips soon
There was a problem hiding this comment.
Hm... makes sense. I'd like to fix this if it's not too much work - for keyboard navigation and stuff to work well. Will see though, later. lots to do.
| }); | ||
| } | ||
|
|
||
| itemsAsValuePaths({ includeRoot = false }) { | ||
| uid(): string { | ||
| return `${this.root}--${this.edges.map((edge) => edge.uid()).join("--")}`; |
There was a problem hiding this comment.
Maybe / for separators instead of --?
Also, it's still possible to create non-unique uids with this code:
So you need to escape () in SqValuePathEdge.uid().
There was a problem hiding this comment.
Happy to use /.
Good catch. I wasn't trying too hard to force non-unique IDs here, but happy to improve it like so.
| } | ||
|
|
||
| // Checks if this SqValuePath completely contains all of the nodes in this other one. | ||
| contains(smallerItem: SqValuePath) { |
There was a problem hiding this comment.
startsWith(prefix: SqValuePath)?
There was a problem hiding this comment.
How about hasAsPrefix or hasPrefix
There was a problem hiding this comment.
That works too. I suggested startsWith because of String.startsWith.
|
|
||
| export function pathIsEqual(path1: SqValuePath, path2: SqValuePath) { | ||
| return pathAsString(path1) === pathAsString(path2); | ||
| export function pathToDisplayString(path: SqValuePath) { |
There was a problem hiding this comment.
Could be path.toDisplayString()
(or path.toString(), see my other comment)
There was a problem hiding this comment.
The annoying thing here is that in utils, we use different names for the root elements than in squiggle-lang. I could do a regex to replace, but that seemed hacky.
Maybe we could rename them in squiggle-lang to match Squiggle-viewer? "Bindings" -> "Variables", capitalize all of them?
There was a problem hiding this comment.
Hmm. Yeah, I missed that this depends on topLevelName.
The problem with renaming is that roots also match keys in SqOutput dicts, and it's nice even if we don't rely on it anywhere.
In export type ValuePathRoot = "result" | "bindings" | "imports" | "exports", I thought about suggesting export type ValuePathRoot = keyof SqOutput, but they might diverge at some point (output could include other, non-value things), so I'm not sure if that's a good idea.
Using public names in SqOutput is too much (and problematic re: versioned-components), so... I dunno. Maybe just leave it here for now.
| }); | ||
|
|
||
| for (const subValuePath of subValuePaths) { | ||
| const pathEdge = subValuePath.lastItem()!; // We know it's not empty, because includeRoot is false. |
There was a problem hiding this comment.
Some time ago I converted this function to traverse the path items manually, instead of pre-generating all prefixes, because of a habit to fight all O(N^2) that I encounter; now you converted it back...
I guess because you disliked C-style for(;;) loop and the way I constructed calculatorPath? Oh well.
The real wart on this function, though, is that it depends on itemStore. Otherwise it would belong to SqValue as a method.
Maybe we can still move it there?
class SqAbstractValue {
...
getSubvalueByPath(
path: SqValuePath,
traverseCalculatorEdge: (path: SqValuePath) => SqValueResult
) {
...
}You could even try to split the implementation into specific SqValue classes by implementing another function, getPathEdge(edge: SqValuePathEdge) (but I'm not sure how to handle calculators with this)
| @@ -68,7 +74,7 @@ export function wrapValue(value: Value, context?: SqValueContext) { | |||
| } | |||
| } | |||
|
|
|||
| export abstract class SqAbstractValue<Type extends string, JSType> { | |||
| export abstract class SqAbstractValue<Type extends string, JSType, ValueType> { | |||
There was a problem hiding this comment.
This extra type seemed safe to have. I think I needed it for getSubvalueByPath at some point - not sure if I need it now, but it seems good to have anyway.
There was a problem hiding this comment.
If you use it only for value(), I don't think it's useful.
If you had a value that's proven to be a specific SqValue (e.g. SqNumberValue), you could extract this:
OTOH, if you add a type parameter like this, then you have to provide it in every subclass, when previously TypeScript would happily infer it for you.
(There's still no partial type argument inference in TypeScript)
There's a longer sub-discussion here about when we should explicitly provide types that could be inferred.
I've noticed that you prefer to write out types more often than I do.
I generally like to rely on type inference, except for cases when I want to guarantee exhaustiveness or have a function with multiple branches.
Example:
function convertToDist(x: Foo) {
if (x.type === "a") {
return x.asDist();
} else if (x.type === "b") {
return processXOfBType(x);
}
}In this case, I'd be tempted to type convertToDist(x: Foo): Dist explicitly, if either of these is true:
- I don't want to write an extra
else { throw new Error(``Unknown x type ${x.type satisifies never}`` }, and want to guarantee that this code won't break whenxgets another possible type - I'm not confident that
processXOfBTypereturns a dist (though then I'd prefer to make that function more reliable and leave this function as is)
In most other cases, I think implicit type inference is a better choice, especially if the return type is complex, which it tends to be, sometimes.
When you have to write something like : result<Dist, SqError> on all functions that pass that result around, that's basically copy-paste, and you would have to change the code in many more locations when you need to edit it.
Btw, if you prefer to write explicit types because hovering over values is annoying, there's a recent inlay hints option in VS Code that can show inferred types.
If we do write out types, I also would prefer to do it close to the location that creates the value (functions as a source of truth). With this generic parameter, we have to write ValueType at the first line of class definition, but the source of truth is get value(), which is declared separately.
Btw, my opinion on the value of this would change if you used it in multiple places. If we had not just get value(): ValueType, but also fromValue(v: ValueType), then sure, let's do it.
One last point: I realize that I don't remember why we need separate SqCalculatorValue vs SqCalculator, etc. They're parameterized by the same data, maybe we can simplify it.
* main: AST debugging view Create grumpy-owls-clean.md tooltips on import strings (injectable through renderImportTooltip) simplify some code ⬆️ Bump vite from 5.0.10 to 5.0.12
I think this is decent, minus the test.