-
Notifications
You must be signed in to change notification settings - Fork 383
Send source file ranges over to the julia engine #13401
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
Conversation
🎉 Snyk checks have passed. No issues have been found so far.✅ security/snyk check is complete. No issues have been found. (View Details) ✅ license/snyk check is complete. No issues have been found. (View Details) |
I think this looks good to merge once tests pass; thanks, @jkrumbiegel! I will say that we are planning a large move towards allowing extensions to contribute engines, and we will probably need to work on a more comprehensive solution to encode our MappedString structures across process boundaries. SourceLocation is a simple encoding of that, but we probably want something richer. (We'll be in touch when we're ready to have you folks work on it, I just wanted to give you a heads up.) |
The new test is passing with my dev branch PumasAI/QuartoNotebookRunner.jl#339 so once that is merged and tagged I can update the PR with the new QNR version here and then it should be good to go. |
cc @gordonwoodhull, just so you're aware of how |
@@ -0,0 +1,3 @@ | |||
```{julia} | |||
"$(@__FILE__):$(@__LINE__)" |
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.
It's really great that you can do this in the julia
engine!
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.
Looks great.
# QuartoNotebookRunner = "=0.17.3" | ||
|
||
[sources] | ||
QuartoNotebookRunner = {url = "https://github.com/PumasAI/QuartoNotebookRunner.jl", rev = "jk/source-ranges"} |
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.
@cscheid this shouldn't have gone in until I have time to review that PR, otherwise quarto master is using a reference to a branch that will change/get deleted soon.
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.
Ah, I'm sorry. We do try to keep main
working (and I will no longer merge these before your ok!).
With that said, we can tolerate some minor amount of breakage here. Can you let me know when the new release is up so we can just bump QNR to (I assume) 0.17.4?
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.
Might get to it this week, might not though, so it's probably best to just revert for the time being.
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.
Got it. But just to understand: in the meantime, the branch exists and presumably works (since the tests passed?)
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.
But just to understand: in the meantime, the branch exists and presumably works (since the tests passed?)
Correct, it'll keep working until we go and merge and delete the branch then stuff here will begin failing.
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.
In that case, I think I'm comfortable with this living on main
for the time being. We're very early on in 1.9; this looks safe to me!
This PR sends the location info for the source files of the
target.markdown
field over to the julia engine so it can send the correct stack traces back. This is the quarto-side part of the fix for #13400 for the julia engine.Checklist
I have (if applicable):