Skip to content

Conversation

jkrumbiegel
Copy link
Contributor

@jkrumbiegel jkrumbiegel commented Jan 7, 2025

Description

This PR adds the ability to change the order in which quarto checks if a markdown file or a language is claimed by an engine. This is necessary to allow users of the native julia engine to use it project-wide, without having to specify engine: julia in each notebook's frontmatter. This is because for backwards compatibility, the native julia engine neither claimed jl script files nor julia language blocks in qmd files, so that jupyter would continue to claim them as usual.

Now, the julia engine does claim these properties, however, jupyter also does, and because the engines are checked in order, jupyter will always win by default. However, by specifying engines: ['julia'] in _quarto.yml, the native julia engine is checked first and therefore wins.

This PR therefore fixes #10034 and #11305

Todos

I'm marking this WIP as I haven't added any tests, yet. First, I'd need to know that this approach is valid, for example if the location of the key in the config is correct. If it is, maybe some quarto-internal schema needs to be extended with this key as well, I couldn't quite make sense of that code when I tried to add engines below the project key at first. The top-level doesn't seem to get validated so I implemented the feature there.

Checklist

I have (if applicable):

  • filed a contributor agreement.
  • referenced the GitHub issue this PR closes
  • updated the appropriate changelog in the PR
  • ensured the present test suite passes
  • added new tests
  • created a separate documentation PR in Quarto's website repo and linked it to this PR

@cscheid
Copy link
Collaborator

cscheid commented Jan 7, 2025

Hey, this is going to be an excellent addition to Quarto, thanks!

I'm marking this WIP as I haven't added any tests, yet.

Sounds good, but this is something we'll definitely need tests for.

First, I'd need to know that this approach is valid, for example if the location of the key in the config is correct.

If it is, maybe some quarto-internal schema needs to be extended with this key as well, I couldn't quite make sense of that code when I tried to add engines below the project key at first.

Did you try adding it to src/resources/schema/project.yml. That should have worked. Specifically, I'd have added a top-level entry to the file like so:

- name: engines
  schema:
    arrayOf: string
  description: "..."

@jkrumbiegel jkrumbiegel marked this pull request as ready for review January 8, 2025 12:15
@jkrumbiegel
Copy link
Contributor Author

jkrumbiegel commented Jan 8, 2025

@cscheid I have added tests. I also added an entry to the schema yml file you suggested, although I can't tell if it had an effect on anything. I was already allowed to put the engines key at top level before, so nothing I can observe changed.

From my side this PR is good to go otherwise.

@jkrumbiegel
Copy link
Contributor Author

Small bump :)

@cderv cderv requested a review from cscheid January 14, 2025 14:00
@mcanouil
Copy link
Collaborator

mcanouil commented Jan 18, 2025

Am I mistaken to think this will also fix?

Side note: what about _metadata.yml?

@jkrumbiegel
Copy link
Contributor Author

Am I mistaken to think this will also fix?

It will not directly fix #3157, it does nothing to change the behavior of the engine key in _quarto.yml. But the idea was that this might not be the right fix anyway, because if you use multiple different languages in your project, it wouldn't make sense to set say julia as the engine for all of them. With the reordering, you'd ideally be able to prioritize all engines that matter to you if you use multiple languages.

Anything I can do to move this along @cscheid :) ?

@jkrumbiegel
Copy link
Contributor Author

One more bump :)

@jkrumbiegel
Copy link
Contributor Author

Would be great to get this one into 1.7 if that's still possible :) Please let me know if there's anything I can do to move this PR forward

@cscheid
Copy link
Collaborator

cscheid commented Mar 5, 2025

Yes, totally possible. Sorry, my last month was a mess of travel and personal stuff. Let's definitely merge this ahead of 1.7.

@cscheid
Copy link
Collaborator

cscheid commented Mar 5, 2025

@jkrumbiegel I'm going to merge this so your users can start testing. Do you think you could draft a docs PR at https://github.com/quarto-dev/quarto-web/?

Copy link
Collaborator

@cscheid cscheid left a comment

Choose a reason for hiding this comment

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

This looks good but we will definitely want documentation since it's not otherwise a very discoverable feature.

@cscheid cscheid merged commit 2552b6e into quarto-dev:main Mar 5, 2025
47 checks passed
@jkrumbiegel
Copy link
Contributor Author

@jkrumbiegel I'm going to merge this so your users can start testing. Do you think you could draft a docs PR at https://github.com/quarto-dev/quarto-web/?

Thank you @cscheid! I can definitely add docs to this, I just thought it'd be better to wait for confirmation first :)

Sorry, my last month was a mess of travel and personal stuff.

No problem, I assumed you had a lot going on. Hope my bumps didn't come across as me being annoyed, I am just trying to not let things go stale for too long

@cscheid
Copy link
Collaborator

cscheid commented Mar 5, 2025

@jkrumbiegel you have personal permission from me to bump every PR of yours as often as you want :) We appreciate all your work on Quarto and the julia community, truly!

It really was just a matter of being swamped.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Script files not picked up by julia engine
4 participants