Skip to content
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

Added reload to {{ stage_name }}.qmd #40

Merged
merged 9 commits into from
Nov 4, 2024
Merged

Added reload to {{ stage_name }}.qmd #40

merged 9 commits into from
Nov 4, 2024

Conversation

tschwarzl
Copy link
Contributor

Added the dso-r reload() functionality to the stage template and added additional beginner friendly explanation and more user friendly layout to the template.

Added the dso-r reload() functionality to the stage template and added additional beginner friendly explanation and more user friendly layout to the template.
@tschwarzl tschwarzl requested review from apeltzer and grst October 22, 2024 21:04
Copy link
Contributor

github-actions bot commented Oct 22, 2024

PR Preview Action v1.4.8
Preview removed because the pull request was closed.
2024-11-04 10:13 UTC

@grst
Copy link
Collaborator

grst commented Oct 23, 2024

I'm not a big fan of adding so many instructions to the template. It's a pet peeve of mine if I initialize a template and need to start with deleting stuff.

This kind of information should go to the docs IMO

@tschwarzl
Copy link
Contributor Author

I have removed explanation of functions in the template and modified it so it contains constructive usage tips.

This is important for beginners to start with and those are imho key aspects the users need to be reminded when starting with dso.

In future, template libraries might help with everything. For now, I would recommend to opt for beginner friendliness.

@grst
Copy link
Collaborator

grst commented Oct 24, 2024

I'm just afraid that this info never gets removed, leading to 90% of the reports start with Load the stage-specific 'params.yaml' config using the `read_params(..)` function. and an unnecessary reload().

What about putting everythying in HTML comments <!-- ... -->? Then at least it wouldn't be rendered in the HTML.

@tschwarzl
Copy link
Contributor Author

params$samplesheet does not exist as well. things have to be removed and modified. This applies to the default quarto template in Rstudio as well, which has a plot and other things to showcase.

I think "ready-to-go" template might be something for a template library. For now, I would opt for something which makes beginners avoid the most frustrating mistakes (e.g. understanding interaction of dvc.yaml, params.in.yaml and params.yaml) and how to access them in a Markdown. We can discuss this in person I guess

@grst
Copy link
Collaborator

grst commented Oct 24, 2024

params$samplesheet does not exist as well

this at least results in an error, while an irrelevant text section does not. But yeah, let's discuss that in a meeting, and maybe hear a 3rd opinion.

tschwarzl and others added 2 commits October 25, 2024 09:19
This changes incorporate @grst comments to have a functional template with comments, but a compromise to cover the most important key learning aspects for beginners in the comments.
@tschwarzl
Copy link
Contributor Author

@grst : I have tried to incorporate your wishes that it should be a functional template, but a compromise that there are comments to cover the basic pit falls for beginners who are completely new to DSO, to avoid instant frustrations with the many new things.

I believe this is a compromise which hopefully will please both sides for now. Template libraries in future will hopefully bring more flexibility in future.

Alternatively, we could also think of adding a third option -> bash, quarto and quarto_minimal ? I am happy for your input and discussions

@tschwarzl tschwarzl requested a review from DSchreyer October 25, 2024 07:23
@DSchreyer
Copy link
Collaborator

I see why it can be an advantage for new users to directly get in contact with the use of the params.in.yaml and the dvc.yaml. I don't see it as large of an issue to remove stuff when starting a stage.

Having a minimal version of the quarto stage might be an idea, it might just get confusing. Maybe adding a parameter like --minimal is the way to go

@grst
Copy link
Collaborator

grst commented Oct 25, 2024

Maybe adding a parameter like --minimal is the way to go

Would be very easy to do actually, using Jinja2 templating

{% if not minimal %}
description text
{% endif %}

Shall we go for that?

But then again it adds additional complexity to the CLI and maybe you are right and we should go with the full template, at least for now.

@tschwarzl
Copy link
Contributor Author

I would like to push the minimal template discussion to another issue.
We have to discuss this in more detail what complexity it will bring with possible future template libraries etc.

@grst
Copy link
Collaborator

grst commented Nov 4, 2024

I thought about this again and you are probably right that at least for now it's more important to get users starting quickly than anything else. In that case I prefer the version you proposed initally over the latest version though, so I reverted the last commit.

Will merge this as soon as CI passes.

@grst grst merged commit 0742b69 into main Nov 4, 2024
5 checks passed
@grst grst deleted the dev-r-reload-template branch November 4, 2024 10:13
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.

3 participants