Skip to content

Conversation

@yairchu
Copy link
Contributor

@yairchu yairchu commented Sep 27, 2025

📝 Summary

Fixes #6390

🔍 Description of Changes

  • When parsing tracebacks, in addition to marimo cells replacing the traceback with a link and button, now other files are also wrapped with a clickable div element, which invokes the user's EDITOR to open the file
  • marimo._server.files.OSFileSystem.open_in_editor now supports opening a file in a specific line, which the traceback uses to open the relevant line from the trace

📋 Checklist

  • I have read the contributor guidelines.
  • I have added tests for the changes made
    • I maintained the existing test for processing traceback HTMLs to work with the new changes
  • I have run the code and verified that it works as expected.

Btw (devops)

I tried to follow the instructions according to CONTRIBUTING.md, but:

  • I had to uninstall an older local version of node for things to work, otherwise node resolved to the oldest version I have which was too old?
  • Couldn't get pnpm to work so had to use make fe for an >1min dev feedback cycle. (btw I'm a front-end noob so I might be missing basic things)

@vercel
Copy link

vercel bot commented Sep 27, 2025

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Preview Comments Updated (UTC)
marimo-docs Ready Ready Preview Comment Sep 28, 2025 8:09am

@github-actions
Copy link

github-actions bot commented Sep 27, 2025

All contributors have signed the CLA ✍️ ✅
Posted by the CLA Assistant Lite bot.

@yairchu
Copy link
Contributor Author

yairchu commented Sep 27, 2025

I have read the CLA Document and I hereby sign the CLA

# otherwise it silently opens the terminal in the same window that is
# running marimo.
if editor and not _is_terminal_editor(editor):
args = [path]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can add tests for the arguments generation, maybe by moving it out to a separate function.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extracted it to a separate function. Ideally it could even be in a separate package so various projects which open files in editors would share this logic. I took this snippet from my srcview package which iirc took it from git-mediate.
The main thing to test is that the editors really expect arguments this way but testing for that seems difficult.

@yairchu
Copy link
Contributor Author

yairchu commented Sep 27, 2025

The previously failing CI tests pointed me to the additional tests I needed to maintain so can now say the changes have tests :)

Btw, make test failed for me in some tests but that also happens for me in main (so I don't think it's related to my changes)

Copy link
Contributor

@Light2Dark Light2Dark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hi @yairchu , I think it looks good overall.

By tests, I meant some Python ones to test the [args] generation.
Feel free to implement what you can, we can also help to add stuff and merge it in.

Took editor arguments snippet from my srcview package. Can potentially extract that specific tidbit to a reusable function.
@yairchu
Copy link
Contributor Author

yairchu commented Sep 28, 2025

I fixed the previous CI errors and I think this time this should be good!
Does the fact that it's at "Expected — Waiting for status to be reported" for several hours mean that this time it's not failing quick and it's just the time it takes the CI to run?

@Light2Dark
Copy link
Contributor

there is an error with the precommit, I created another PR for that. Thanks for the contribution!

@yairchu
Copy link
Contributor Author

yairchu commented Sep 28, 2025

Looks like this time the 3 failing CIs aren't this PR's fault:

  • pre-commit already faulty as you mentioned
  • cli test / Tests CLI on windows-latest 3.12: Failed in a timeout that seems totally unrelaters
  • check-schemas (openapi, packages/openapi/api.yaml, OpenAPI specification, true): Error: failed to load base spec from "main/packages/openapi/api.yaml" - the yaml was auto-generated by make fe-codegen?

@mscolnick
Copy link
Contributor

@yairchu this looks great! the failing tests are likely bc you are on a fork and our GH workflows likely don't handle that well, they are just red-herring, but the main tests do pass

@mscolnick mscolnick merged commit ab5b667 into marimo-team:main Sep 28, 2025
37 of 41 checks passed
@yairchu yairchu deleted the traceback-editor-links branch September 28, 2025 22:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bash-focus Area to focus on during release bug bash

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Click filenames in stack trace to open locally in editor

3 participants