OBEE-39: implement table runtime and backing for createInstance factory - #1413
OBEE-39: implement table runtime and backing for createInstance factory#1413tslil-topos wants to merge 1 commit into
Conversation
tslil-topos
added
enhancement
Notion Integration |
4e5cb13 to
c8e5a9f
Compare
c8e5a9f to
3fa7ebb
Compare
| function orderedRowIds(table: Readonly<DocumentTypes.Table> | undefined): string[] { | ||
| if (table === undefined) { | ||
| return []; | ||
| } | ||
| const seen = new Set<string>(); | ||
| const ordered: string[] = []; | ||
| for (const id of table.rowOrder ?? []) { | ||
| if (table.rows[id] !== undefined && !seen.has(id)) { | ||
| seen.add(id); | ||
| ordered.push(id); | ||
| } | ||
| } | ||
| for (const id of Object.keys(table.rows)) { | ||
| if (!seen.has(id)) { | ||
| ordered.push(id); | ||
| } | ||
| } | ||
| return ordered; | ||
| } |
There was a problem hiding this comment.
This is being defensive in the wrong place and also papers over any structural issues if they are there. It speaks to the overall flaw I see in this implementation: we should parse our doc into a structure where we know:
table.rowOrderis not undefinedtable.rowOrderhas all the keys oftable.rowsand no more- any other invariants we want to hold for the JSON structure
We should then be able to use table.rowOrder directly instead of this function. In general it should allow us to split up the process into something like "parse-doc.ts" and then "table-methods.ts" that operate on the parsed doc.
There was a problem hiding this comment.
I think flaw is a strong word. The comments and PR text explain the approach taken: all actions are maximally permissive. If the instance is in a bad state we should still be able to take all actions that we are interested in. It's up to users of this api to ensure that valid passes, and all of these functions share the guarantee: if the schema and instance are valid then all operations are faithful and exhibit all data.
There was a problem hiding this comment.
I don't know what word to use but I disagree with the approach exhibited by this defensive error swallowing code and with things like:
Every update that fails to address is skipped and the rest are still applied.
Where "skipped" means "silently skipped"
There was a problem hiding this comment.
Maybe you could weigh in on this one @epatters since tslil and I seem to have very different views on this.
There was a problem hiding this comment.
I don't know what word to use but I disagree with the approach exhibited by this defensive error swallowing code and with things like:
Every update that fails to address is skipped and the rest are still applied.
Where "skipped" means "silently skipped"
it does not, did you see the preceding sentence or the code paths Only addressing failures are reported as issues.?
There was a problem hiding this comment.
Ok, looks good on re-reading. You pushed some updates to comments?
I still think that the function orderedRowIds is not the right approach. I'll open a PR for how I think it can be improved.
3fa7ebb to
d917c95
Compare
|
@kasbah please approve this change so that we can proceed to the difficult and time-consuming integration work that lives on-top of this stack. If we want to revisit this approach, assuming that this doesn't break fundamental UI operations (would be happy to hear a case that the split of error reporting between addressing issues on operations, all issues on validate is difficult for consumers), then we can do so after the deadline and we could open an issue now to remind us to do so once there is more time. |
*tests will be included in the last change stacked on this*
This is an absolute grind of a change. There are simply twelve quadrillion things that have to happen to make things like changing rows or adjusting tables work correctly.
The basic pattern is visible in instance-runtime.ts :
```typescript
async function withValidatedSchema<
Handle,
S extends Shape,
T,
E extends ReadonlyArray<Issue> = ReadonlyArray<Issue>,
>(
schema: Notebook<S, ModelDocument>,
operation: (schemaModel: DblModel) => Result<T, E>,
): Promise<Result<T, E>>
```
is used to perform the steps of obtaining a dblmodel (or erroring if the schema doesn't validate), learning about the table structure, and finally running the operation (which might itself fail in a way at least as exciting as ReadonlyArray<Issue>). So this is what is contained in the factory functions that will (in a later change) back the Instance factory: `createTablesMethod`, `createGetMethod`, `createAddRowManyMethod`, etc.
Then there's the elephant: table-methods.ts. The basic premise for all of these is to permissively apply everything that makes sense, and report everything that failed. Thus an `Ok` is only possible if went well. The only kinds of failures that are important here are addressing failures: mentioning a table that doesn't exist, mentioning a row that doesn't exist. Type issues, missing entries, bad links, and so on are all semantic checks that will be caught by validation passes.
d917c95 to
0a70f9e
Compare
2 participants
tests will be included in the last change stacked on this
This is an absolute grind of a change. There are simply twelve quadrillion things that have to happen to make things like changing rows or adjusting tables work correctly.
The basic pattern is visible in instance-runtime.ts :
is used to perform the steps of obtaining a dblmodel (or erroring if the schema doesn't validate), learning about the table structure, and finally running the operation (which might itself fail in a way at least as exciting as ReadonlyArray). So this is what is contained in the factory functions that will (in a later change) back the Instance factory:
createTablesMethod,createGetMethod,createAddRowManyMethod, etc.Then there's the elephant: table-methods.ts. The basic premise for all of these is to permissively apply everything that makes sense, and report everything that failed. Thus an
Okis only possible if went well. The only kinds of failures that are important here are addressing failures: mentioning a table that doesn't exist, mentioning a row that doesn't exist. Type issues, missing entries, bad links, and so on are all semantic checks that will be caught by validation passes.