-
Notifications
You must be signed in to change notification settings - Fork 382
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: top level functions #3755
base: main
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is excellent. Saw the attention to detail with preserving comments in various locations — between the decorator invocation and function def, in the function body ... and really appreciate that. My only note is what you already called out, to rename app.fn
to app.function
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome! LGTM modulo failing tests, looks like just snapshots that need to be updated.
Weird, no idea what these failures are. They don't seem related ...
|
I'm also unable to reproduce the On the surface it feels like it would be related to this PR, but I also see it failing in another PR ... https://github.com/marimo-team/marimo/actions/runs/13278229043/job/37071591905?pr=3760 |
📝 Summary
Adds top level functions
@app.fn
(but maybeapp.function
is better) to wrap functions, register them but leave them directly exposed@app.fn
if there are no dependenciesJust an intro PR for #2293
For completion of 2293, I think out standing changes are:
But this is hidden under
App(_toplevel_fn=True)
🔍 Description of Changes
see the smoke test or:
📋 Checklist
📜 Reviewers
@akshayka OR @mscolnick