Error UI changes j 2023 #2034

OAGr · 2023-07-11T16:46:18Z

Before:

After:

changeset-bot · 2023-07-11T16:46:21Z

⚠️ No Changeset found

Latest commit: d532204

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

vercel · 2023-07-11T16:46:24Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Updated (UTC)
quri-hub	❌ Failed (Inspect)	Jul 29, 2023 0:40am
quri-ui	❌ Failed (Inspect)	Jul 29, 2023 0:40am
squiggle-components	❌ Failed (Inspect)	Jul 29, 2023 0:40am
squiggle-website	❌ Failed (Inspect)	Jul 29, 2023 0:40am

berekuk

I understand that adding SqType can be a lot of (easy, but annoying and repetitive) work.

So while I think that we should do it the way I suggest in my comment, we can merge this without it and clean it up some time later.

berekuk · 2023-11-20T18:04:26Z

packages/squiggle-lang/package.json

@@ -29,6 +29,7 @@
  },
  "dependencies": {
    "@commander-js/extra-typings": "^11.0.0",
+    "@tailwindcss/typography": "^0.5.9",


squiggle-lang shouldn't depend on tailwind, I assume (hope) that you installed it here instead of components by mistake.

I probably did, this was done quickly

berekuk · 2023-11-20T19:02:58Z

packages/components/src/components/SquiggleErrorAlert.tsx

-    </span>
+    <div className="inline-block">
+      <span
+        className="text-xs text-blue-500 cursor-pointer items-center flex border-b border-blue-700 border-opacity-50 hover:bg-red-200 leading-4"


(minor)

This html/css is more complicated than necessary, I think (haven't checked, builds are failing). inline-block not needed because parent is flexbox; leading-4 is weird too because it's always (?) a single line, vertical alignment is done with items-center on parent.

Maybe it's a consequence of using borders instead of underline? There's enough classes in Tailwind to style underline properly (https://tailwindcss.com/docs/text-decoration-color, https://tailwindcss.com/docs/text-underline-offset), though I'd just suggest something simpler and more similar to StyledLink, like hover:underline text-blue-500 decoration-dashed.

berekuk · 2023-11-20T19:04:25Z

packages/components/src/components/SquiggleErrorAlert.tsx

  );
 };
+console.log(style, "STLYYE");


Suggested change

console.log(style, "STLYYE");

berekuk · 2023-11-20T19:07:01Z

packages/components/src/components/SquiggleErrorAlert.tsx

+                    {...props}
+                    language={match[1]}
+                    PreTag="div"
+                    style={style["oneDark"]}


Dark theme looks too intense to me, I haven't checked how it looks in the context of playground since builds are failing but I expect it would be distracting.

berekuk · 2023-11-20T19:25:01Z

packages/squiggle-lang/src/value/index.ts

@@ -25,6 +25,10 @@ abstract class BaseValue {
    return Object.assign(Object.create(Object.getPrototypeOf(this)), this);
  }

+  toTypeString(): string {


I think you ended up not using this?

If you did, I'd be opposed to it, because generally it's not possible to infer type from a value; e.g. [3,4,5] type should be number[], not [number, number, number].

berekuk · 2023-11-20T19:52:50Z

packages/squiggle-lang/src/library/registry/core.ts

+\`\`\`js
+${defs}
+\`\`\`
+Was given **\`${givenFn})\`**`;


There are some things in this file and your overall approach that I think we should do differently.

You serialize function definitions to parse them again later (with JS parser, but that part could be improved) on the frontend. This is lossy and fragile, and I suggest we expose full type definitions instead.

After this PR you already did changes that added SqLambdaParameter. On main branch, SqLambdaParameter exposes only typeName?: string, but we could extend it to expose SqType that would wrap FRType objects. Then, on the frontend, we could render it by recursively turning it into JSX (similar to SquiggleViewer, but much easier), with whatever styling we need.

This is not 100% straightforward because right now FrType doesn't expose tag like SqValue does, so you can't switch over it, but this can be changed.

Type definitions will also be useful in other places, not just stacktraces:

in autocompletion menu:

on hover in editor when we add type inference for user-defined values;

in docs (it's possible to replace manually defined types in docs with <RegistryFunctionSignatures fn="Plot.dist" /> component for Nextra, if we expose function registry from squiggle-lang)

This also means that we'd have to wrap ErrorMessage in SqErrorMessage, with abstract base class and discriminated union over specific sub-classes; that seems useful to me, as we previously discussed that different error messages should have different fields, otherwise a single REOther would be enough for everything.

ErrorMessage could still be serializable to a string, e.g. for CLI; it's just that on web I think we shouldn't do object->string->parsed string conversion.

berekuk · 2023-11-21T17:23:36Z

packages/squiggle-lang/src/errors/messages.ts

I'm on the fence about markdown in toString. errorTypeName seems good, but markdown would look weird in CLI.

Thoughts:

maybe we could render markdown (at least bold/italic) in CLI by using a formatter that turns bold and italic markup to color?

I still believe that we probably have too many ErrorMessage subclasses, and a single class with (message, errorType) constructor would be enough in most cases

it's easier to create a full message at the location that raises an error

multiple ErrorMessage subclasses are useful only if they have different fields (if we adopt my proposal about SqType from another comment)

berekuk · 2023-11-21T17:27:48Z

packages/squiggle-lang/src/public/SqError.ts

@@ -18,6 +19,10 @@ export class SqFrame {
  location() {
    return this._value.location;
  }
+
+  toTypeString(): string[] {


SqFrame doesn't extend SqAbstractError, and this method is unused.

berekuk · 2023-11-21T17:34:06Z

packages/squiggle-lang/src/public/SqError.ts

@@ -6,6 +6,7 @@ abstract class SqAbstractError<T extends string> {

  abstract toString(): string;
  abstract toStringWithDetails(): string;
+  abstract toTypeString(): string[];


Allowing arbitrary number of type strings here seems excessive, or maybe I don't understand the intention; I see that you join everything except the first item with dashes in components, and I'm not sure if you had some longer-term vision about when more than 2 elements would be useful.

The way I'd do it is something like:

abstract getType(): string for top-level error type; or, even better, change tag field to match that public name and use that in components

separate getSubtype(): string { return ""; }, with "" as a default and overridden in SqRuntimeError.

"" instead of undefined for the same reason as in Saving variableType and docstrings #2542 (comment)

(I'm using get prefix instead of to prefix in both cases, as to suggests "represent an entire error object as a string", while type and subtype are parts of the object)

OAGr · 2023-11-22T20:51:57Z

Looking at this again, I think it was trying to do too much for one PR. (This does make sense, it was experimental).

I think I might isolate some of the simplest parts for a PR to do soon, then might do some of it incrementally.

berekuk · 2024-03-27T19:22:27Z

This PR seems too stale to merge now, can we close it?

OAGr added 2 commits July 10, 2023 22:25

Show wrong argument types

81f422e

Error should show error type, and has better styling

80e06f2

Merged with develop

3f18b54

vercel bot deployed to Preview – quri-ui July 27, 2023 00:46 View deployment

vercel bot deployed to Preview – squiggle-website July 27, 2023 00:50 View deployment

vercel bot deployed to Preview – quri-hub July 27, 2023 00:53 View deployment

vercel bot deployed to Preview – squiggle-components July 27, 2023 00:54 View deployment

Adding markdown error messages

a9439a6

vercel bot deployed to Preview – quri-ui July 27, 2023 15:36 View deployment

vercel bot had a problem deploying to Preview – quri-hub July 27, 2023 15:37 Failure

vercel bot had a problem deploying to Preview – squiggle-components July 27, 2023 15:38 Failure

vercel bot had a problem deploying to Preview – squiggle-website July 27, 2023 15:38 Failure

Merged with main

e444e94

vercel bot had a problem deploying to Preview – squiggle-website July 28, 2023 22:11 Failure

vercel bot deployed to Preview – quri-ui July 28, 2023 22:11 View deployment

vercel bot had a problem deploying to Preview – quri-hub July 28, 2023 22:12 Failure

vercel bot had a problem deploying to Preview – squiggle-components July 28, 2023 22:12 Failure

Mild markdown formatting

d532204

vercel bot had a problem deploying to Preview – squiggle-website July 29, 2023 00:39 Failure

vercel bot had a problem deploying to Preview – quri-hub July 29, 2023 00:39 Failure

vercel bot had a problem deploying to Preview – quri-ui July 29, 2023 00:39 Failure

vercel bot had a problem deploying to Preview – squiggle-components July 29, 2023 00:40 Failure

berekuk reviewed Nov 21, 2023

View reviewed changes

OAGr mentioned this pull request Dec 5, 2023

Error alert improvements #2662

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Error UI changes j 2023 #2034

Error UI changes j 2023 #2034

OAGr commented Jul 11, 2023 •

edited

Loading

changeset-bot bot commented Jul 11, 2023 •

edited

Loading

vercel bot commented Jul 11, 2023 •

edited

Loading

berekuk left a comment

berekuk Nov 20, 2023

OAGr Nov 21, 2023

berekuk Nov 20, 2023

berekuk Nov 20, 2023

berekuk Nov 20, 2023

berekuk Nov 20, 2023

berekuk Nov 20, 2023

berekuk Nov 21, 2023

berekuk Nov 21, 2023

berekuk Nov 21, 2023

OAGr commented Nov 22, 2023

berekuk commented Mar 27, 2024

Error UI changes j 2023 #2034

Are you sure you want to change the base?

Error UI changes j 2023 #2034

Conversation

OAGr commented Jul 11, 2023 • edited Loading

changeset-bot bot commented Jul 11, 2023 • edited Loading

⚠️ No Changeset found

vercel bot commented Jul 11, 2023 • edited Loading

berekuk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OAGr commented Nov 22, 2023

berekuk commented Mar 27, 2024

OAGr commented Jul 11, 2023 •

edited

Loading

changeset-bot bot commented Jul 11, 2023 •

edited

Loading

vercel bot commented Jul 11, 2023 •

edited

Loading