Simple Squiggle Calculators #2265
Conversation
🦋 Changeset detectedLatest commit: c030b44 The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Ignored Deployment
|
Codecov ReportPatch coverage has no change and project coverage change:
Additional details and impacted files@@ Coverage Diff @@
## main #2265 +/- ##
==========================================
- Coverage 73.04% 72.32% -0.72%
==========================================
Files 107 109 +2
Lines 5349 5405 +56
Branches 1035 1051 +16
==========================================
+ Hits 3907 3909 +2
- Misses 1435 1489 +54
Partials 7 7 ☔ View full report in Codecov by Sentry. |
|
Some changes to make:
Potential changes for future PRs:
|
| @@ -0,0 +1,212 @@ | |||
| import React, { Reducer } from "react"; | |||
There was a problem hiding this comment.
Note: I've moved much of this functionality to ViewerProvider, so this file now isn't used much.
* main: another publish fix attempt to fix publish script Version Packages build squiggle-lang on releases update-system-version in changeset-version update-system-version npm script publish to npm and vscode marketplace correct vscode ext link filter out "Patched changes" entries update _meta.json changeset-version script split Changelog into multiple pages fix hydration error remove unused combine-dependabot workflow fix codeql workflow
There was a problem hiding this comment.
I'll clean up things around Calculator and CalculatorUI myself (with custom hooks; linter warnings are concerning).
Rest of the comments flag a few minor things, nothing that would block the merge.
One more thing for the future is that I think there might be potential race conditions in asyncActions, but that's not important right now because calculator runs are synchronous.
Congrats on getting through this!
| @@ -17,6 +17,7 @@ export default { | |||
| }, | |||
| plugins: [ | |||
| require("@quri/squiggle-components/tailwind-plugin"), | |||
| require("@tailwindcss/typography"), | |||
There was a problem hiding this comment.
Any addition to this list requires an update to the components README (Styles, section 1 and 3).
| @@ -15,7 +15,16 @@ Raw functions and distributions are plotted with default parameters, while plot | |||
| Plots one or more labeled distributions on the same plot. Distributions can be either continuous, discrete, or a single number. | |||
|
|
|||
| ``` | |||
| Plot.dists: ({dists: list<{name: string, value: distribution | number}>, xScale: scale, yScale: scale, title: string, showSummary: bool}) => plot | |||
| Plot.dists: ({ | |||
There was a problem hiding this comment.
Unrelated changes, unrelated comment: I wish this was a real syntax. Hope we'll get there eventually, when we add more kinds of parameter annotations.
| @@ -41,9 +41,10 @@ export class SqValueContext { | |||
| let ast = this.valueAst; | |||
|
|
|||
| let newAst: ASTNode | undefined; | |||
| const itemisNotTableIndex = item.type !== "cellAddress"; | |||
| const itemisNotTableIndexOrCalculator = | |||
There was a problem hiding this comment.
Semantic name might be better, e.g. isExtendable.
|
|
||
| export type MergedItemSettings = PlaygroundSettings; | ||
|
|
||
| export const pathItemFormat = (item: PathItem): string => { | ||
| if (item.type === "cellAddress") { | ||
| return `Cell (${item.value.row},${item.value.column})`; | ||
| } else if (item.type === "calculator") { | ||
| return `calculator`; |
There was a problem hiding this comment.
Suggestion: either capitalized "Calculator", or some other notation that's not a valid variable name.
Then there'd be no ambiguity here:
There was a problem hiding this comment.
Good point. I imagine we'll also iterate on this more over time (maybe add an icon or something with it)
| @@ -107,14 +110,19 @@ export class SqValuePath { | |||
| }); | |||
| } | |||
|
|
|||
| itemsAsValuePaths() { | |||
| return this.items.map( | |||
| itemsAsValuePaths({ includeRoot = false }) { | |||
There was a problem hiding this comment.
Whoa, i didn't know that this is allowed and type-safe.
I usually do fn({ param = false }: { param?: boolean }).
You'd still have to do that if you want to allow fn() instead of fn({}), though.
| // This is the one that we use in the ProviderContext to store information about the calculator. | ||
| const allItems = pathItem.itemsAsValuePaths({ includeRoot: true }); | ||
| const previousPathItem: SqValuePath | undefined = | ||
| allItems.length > 1 ? allItems[allItems.length - 2] : undefined; |
There was a problem hiding this comment.
allItems.at(-2) is a nice shortcut for this.
| ): SqValue | undefined { | ||
| // The previous path item is the one that is the parent of the calculator result. | ||
| // This is the one that we use in the ProviderContext to store information about the calculator. | ||
| const allItems = pathItem.itemsAsValuePaths({ includeRoot: true }); |
There was a problem hiding this comment.
itemsAsValuePaths is quadratic on path length, and this function is called from extractSubvalueByPath which has a loop over itemsAsValuePaths too.
I checked that there's no O(N^4) here, unless N is the number of nested calculators, so it's fine, but it still makes me slightly nervous (what if we decide to call extractSubvalueByPath on each subpath, for some reason, or something like that?)
There was a problem hiding this comment.
Should be CalculatorUI.tsx, to match the component name.
(it's somewhat hard to change on macOS, the best way I know is to git mv {c,C}alculatorUI.tsx and then to reopen in VS Code or even restart it).
| )} | ||
| </div> | ||
| calculator && | ||
| calculatorState && ( |
There was a problem hiding this comment.
Aren't both of these always defined?
|
Okay, merging now, can clean up more later. |
No description provided.