Skip to content

Improve pre/post render script logging #12444

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

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Conversation

cderv
Copy link
Collaborator

@cderv cderv commented Apr 3, 2025

This is an idea coming from work on

The logging was not completely explicit as it was showing on script name without context.

This PR adds Running script before.

Also it introduces an idea: Set a new env var so that the pre/post render script know when then need to make there option console.log quiet.

If this seems a good idea, I can add test.

Comment on the process

I initially did some commit to add the env var with others

const prePostEnv = {
"QUARTO_PROJECT_OUTPUT_DIR": projOutputDir,
...(projectRenderConfig.behavior.renderAll
? { QUARTO_PROJECT_RENDER_ALL: "1" }
: {}),
};

but in fact, it seems better to defined closer to its meaning,

if (progress && !quiet) {
info(colors.bold(colors.blue(`Running script '${script}'`)));
env["QUARTO_PROJECT_SCRIPT_PROGRESS"] = "1";
}

which leads to

  • QUARTO_PROJECT_SCRIPT_PROGRESS is set to 1 means progress can be shown
  • QUARTO_PROJECT_SCRIPT_PROGRESS is unset, means no progress is expected

@cderv cderv force-pushed the post-render/logging branch 2 times, most recently from d5dc015 to c5a2169 Compare April 3, 2025 17:27
@cscheid cscheid modified the milestones: v1.7, v1.8 Apr 7, 2025
@cscheid
Copy link
Collaborator

cscheid commented Apr 8, 2025

@cderv I'm pushing this to v1.8 but feel free to move back to v1.7 if you've made the changes we talked about last week.

@cderv cderv force-pushed the post-render/logging branch from c5a2169 to 45c8580 Compare April 8, 2025 18:55
@cderv
Copy link
Collaborator Author

cderv commented Apr 8, 2025

I rebased - this is where I was.

@cscheid
Copy link
Collaborator

cscheid commented Apr 8, 2025

Right - we had talked about using QUARTO_QUIET and QUARTO_PROGRESS instead, and making that part of the codebase less branchy. I think that would be a worthy change (and doesn't need to happen for 1.7).

@cderv
Copy link
Collaborator Author

cderv commented Apr 9, 2025

QUARTO_QUIET and QUARTO_PROGRESS instead

Yeah I used more verbose env var name because I feel those two are too generic... We are in pre-render post-render script context here, so I wanted to scope the variable name as it is for QUARTO_PROJECT_OUTPUT_DIR

It does not apply to quarto run for example. So it seems there topic here is to make env var available in a clever way, wherever they are supposed to (e.g quarto run, pre-render, post-render, execution ? Lua filters ?)

A border topic.

making that part of the codebase less branchy.

I have done that part by removing the if else and using a default handler.run. Did you have in mind something more generic like making quarto run and pre/post render sharing even more logic ?

Let's continue in 1.8 then.

@cderv cderv self-assigned this Apr 9, 2025
@cderv cderv added the early-in-release An issue that should be worked on early in the release (likely due to risk) label Apr 9, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
early-in-release An issue that should be worked on early in the release (likely due to risk)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants