feat: auto-import marimo for markdown #1404
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
There was a problem hiding this comment.
To avoid a flash of NameError -> new cell -> re-running the markdown, I suggest specializing this only to markdown actions in the frontend, and doing the logic in the frontend as well, without running any code.
When the user clicks "Create Markdown" or "View as Markdown" ...
- Check for
import[ \t]+marimo[ \t]+as[ \t]+moacross all cells. - Check for
moin the variables table. - If
import marimois not found, andmois not in the variables table, then add a new cell withimport marimo as moto the top of the notebook. Ifautorun on startupis enabled, run it. - Add the markdown cell.
1 and 2 should be inexpensive operations, so I wouldn't worry about overhead.
This hotfix is really only needed for markdown anyway. When typing Python, a NameError for mo is readily understood and fixed by the user.
mscolnick
force-pushed
the
ms/add-missing-import
branch
from
61826bd
to
0db44bd
Compare
mscolnick
changed the title
mscolnick
force-pushed
the
ms/add-missing-import
branch
from
0db44bd
to
c702899
Compare
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @mscolnick - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 3 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
| @@ -22,708 +22,722 @@ import { createReducerAndAtoms } from "../../utils/createReducer"; | |||
| import { arrayInsert, arrayDelete } from "@/utils/arrays"; | |||
| import { foldAllBulk, unfoldAllBulk } from "../codemirror/editing/commands"; | |||
| import { mergeOutlines } from "../dom/outline"; | |||
| import { CellHandle } from "@/components/editor/Cell"; | |||
There was a problem hiding this comment.
suggestion: Use of 'type' keyword for imports
The use of the 'type' keyword for importing types is a good practice as it helps in distinguishing between value imports and type imports. This can also potentially reduce the bundle size if the type is not used at runtime.
| import { Logger } from "@/utils/Logger"; | ||
| import { Objects } from "@/utils/objects"; | ||
| import { splitAtom, selectAtom } from "jotai/utils"; | ||
| import { isStaticNotebook, parseStaticState } from "../static/static-state"; | ||
| import { CellLog, getCellLogsForMessage } from "./logs"; |
There was a problem hiding this comment.
suggestion: Use of 'type' keyword for imports
The use of the 'type' keyword for importing types is a good practice as it helps in distinguishing between value imports and type imports. This can also potentially reduce the bundle size if the type is not used at runtime.
| import { CellLog, getCellLogsForMessage } from "./logs"; | |
| import { type CellLog } from "./logs"; | |
| import { getCellLogsForMessage } from "./logs"; |
| import { deserializeBase64, deserializeJson } from "@/utils/json/base64"; | ||
| import { historyField } from "@codemirror/commands"; | ||
| import { clamp } from "@/utils/math"; | ||
| import { LayoutState } from "../layout/layout"; |
There was a problem hiding this comment.
suggestion: Use of 'type' keyword for imports
The use of the 'type' keyword for importing types is a good practice as it helps in distinguishing between value imports and type imports. This can also potentially reduce the bundle size if the type is not used at runtime.
frontend/src/core/cells/cells.ts
Outdated
| }; | ||
| }, | ||
| deleteCell: (state, action: { cellId: CellId }) => { | ||
| const cellId = action.cellId; |
There was a problem hiding this comment.
suggestion (code-quality): Prefer object destructuring when accessing and using properties. (use-object-destructuring)
| const cellId = action.cellId; | |
| const {cellId} = action; |
Explanation
Object destructuring can often remove an unnecessary temporary reference, as well as making your code more succinct.From the Airbnb Javascript Style Guide
frontend/src/core/cells/cells.ts
Outdated
| }, | ||
| scrollToTarget: (state) => { | ||
| // Scroll to the specified cell and clear the scroll key. | ||
| const scrollKey = state.scrollKey; |
There was a problem hiding this comment.
suggestion (code-quality): Prefer object destructuring when accessing and using properties. (use-object-destructuring)
| const scrollKey = state.scrollKey; | |
| const {scrollKey} = state; |
Explanation
Object destructuring can often remove an unnecessary temporary reference, as well as making your code more succinct.From the Airbnb Javascript Style Guide
frontend/src/core/cells/cells.ts
Outdated
| const errors = cellIds | ||
| .map((cellId) => { | ||
| const cell = cellRuntime[cellId]; | ||
| const { name } = cellData[cellId]; | ||
| if (cell.output?.mimetype === "application/vnd.marimo+error") { | ||
| // Filter out ancestor-stopped errors | ||
| // These are errors that are caused by a cell that was stopped, | ||
| // but nothing the user can take action on. | ||
| const nonAncestorErrors = cell.output.data.filter( | ||
| (error) => error.type !== "ancestor-stopped", | ||
| ); | ||
|
|
||
| if (nonAncestorErrors.length > 0) { | ||
| return { | ||
| output: { ...cell.output, data: nonAncestorErrors }, | ||
| cellId: cellId, | ||
| cellName: name, | ||
| }; | ||
| } | ||
| } | ||
|
|
||
| return null; | ||
| }) | ||
| .filter(Boolean); | ||
| return errors; |
There was a problem hiding this comment.
suggestion (code-quality): Inline variable that is immediately returned (inline-immediately-returned-variable)
| const errors = cellIds | |
| .map((cellId) => { | |
| const cell = cellRuntime[cellId]; | |
| const { name } = cellData[cellId]; | |
| if (cell.output?.mimetype === "application/vnd.marimo+error") { | |
| // Filter out ancestor-stopped errors | |
| // These are errors that are caused by a cell that was stopped, | |
| // but nothing the user can take action on. | |
| const nonAncestorErrors = cell.output.data.filter( | |
| (error) => error.type !== "ancestor-stopped", | |
| ); | |
| if (nonAncestorErrors.length > 0) { | |
| return { | |
| output: { ...cell.output, data: nonAncestorErrors }, | |
| cellId: cellId, | |
| cellName: name, | |
| }; | |
| } | |
| } | |
| return null; | |
| }) | |
| .filter(Boolean); | |
| return errors; | |
| return cellIds | |
| .map((cellId) => { | |
| const cell = cellRuntime[cellId]; | |
| const { name } = cellData[cellId]; | |
| if (cell.output?.mimetype === "application/vnd.marimo+error") { | |
| // Filter out ancestor-stopped errors | |
| // These are errors that are caused by a cell that was stopped, | |
| // but nothing the user can take action on. | |
| const nonAncestorErrors = cell.output.data.filter( | |
| (error) => error.type !== "ancestor-stopped", | |
| ); | |
| if (nonAncestorErrors.length > 0) { | |
| return { | |
| output: { ...cell.output, data: nonAncestorErrors }, | |
| cellId: cellId, | |
| cellName: name, | |
| }; | |
| } | |
| } | |
| return null; | |
| }) | |
| .filter(Boolean); | |
Explanation
Something that we often see in people's code is assigning to a result variableand then immediately returning it.
Returning the result directly shortens the code and removes an unnecessary
variable, reducing the mental load of reading the function.
Where intermediate variables can be useful is if they then get used as a
parameter or a condition, and the name can act like a comment on what the
variable represents. In the case where you're returning it from a function, the
function name is there to tell you what the result is, so the variable name
is unnecessary.
|
🚀 Development release published. You may be able to view the changes at https://marimo.app?v=0.6.1-dev9 |
* feat: auto-import marimo for markdown * format * lint * fixes * ci * fix * fix
This will auto-import
import marimo as moafter toggling markdown