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

Vignettes #43

Merged
merged 11 commits into from
Oct 22, 2024
Merged

Vignettes #43

merged 11 commits into from
Oct 22, 2024

Conversation

tomaszaba
Copy link
Collaborator

Hi Ernest,

In this PR I addressed issues #14 , #15 , #16.
I have also addressed the issue about the use of the verb to wrangle on age.

I look forward to your feedback.

Thanks.

@tomaszaba tomaszaba added the documentation Improvements or additions to documentation label Oct 21, 2024
Copy link
Member

@ernestguevarra ernestguevarra left a comment

Choose a reason for hiding this comment

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

@tomaszaba I've looked at your vignettes. Two major comments here:

  • the front matter for the YAML component of the .qmd files need to be updated

This is so that they will be rendered specifically as vignettes particularly in the build setup and in the CRAN setup.

  • vignettes directory should only contain the vignette documents.

In your commit, you added the directories created when building the vignette. These will be flagged by CRAN as unrecognised directories/files.

Instead of reviewing this, I will add to this PR to make it work for CRAN (if possible) and to make it work with pkgdown (which we know already have limitations).

I think if we really want to write all these things that you want to include in a vignette, let's make the vignette standard (meaning Rmd) and then you just write a book using Quarto where you can write as long as you want all the guides to using the package.

@ernestguevarra
Copy link
Member

@tomaszaba I've pushed in infrastructural changes to your PR that does the following:

  1. Update the YAML front matter of your qmd vignettes;
  2. setup _pkgdown.yml to style pkgdown website so we can view the vignettes online; this uses nutriverse template
  3. setup pkgodwn build via GitHub Actions.

if you want to build the website, issue the following command on R console:

pkgdown::build_site()

Compare this output to what the website for nipnTK looks like. The nutriverse theme/template is not working and the additional features for pkgdown are not showing...

@ernestguevarra
Copy link
Member

My suggestion is to revert the vignette and README back to Rmd until pkgdown has full Quarto support. Right now, all that we are using Quarto for that is different from Rmd are the callouts and that is not getting implemented properly by pkgdown.

I have read up on others work to make Quarto work for README and vignettes and pkgdown. It is not impossible but it will take time to configure our documentation workflow.

For me what is more important is a more maintainable package documentation that will pass CRAN and easy to maintain. I think we will be updating documentation a lot after we do a pre-release for alpha testing and I can't imagine how much trouble it will be if documentation will take so much effort to update compared to a fully functioning reliable automated system that is already in place.

@tomaszaba
Copy link
Collaborator Author

tomaszaba commented Oct 21, 2024

My suggestion is to revert the vignette and README back to Rmd until pkgdown has full Quarto support. Right now, all that we are using Quarto for that is different from Rmd are the callouts and that is not getting implemented properly by pkgdown.

I have read up on others work to make Quarto work for README and vignettes and pkgdown. It is not impossible but it will take time to configure our documentation workflow.

For me what is more important is a more maintainable package documentation that will pass CRAN and easy to maintain. I think we will be updating documentation a lot after we do a pre-release for alpha testing and I can't imagine how much trouble it will be if documentation will take so much effort to update compared to a fully functioning reliable automated system that is already in place.

😬 This hurts to know. Let me put it back. I will have to use the html syntax for the callouts as I had in the previous version of both README and vignettes. I will change README in the same branch.

Copy link
Member

@ernestguevarra ernestguevarra left a comment

Choose a reason for hiding this comment

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

@tomaszaba please merge to main

@tomaszaba
Copy link
Collaborator Author

@tomaszaba please merge to main

@ernestguevarra thanks for approving this PR, however, the "merge pull request" remains disabled due to the failure in one of the GitHub checks. That on windows.

@ernestguevarra
Copy link
Member

@tomaszaba please merge to main

@ernestguevarra thanks for approving this PR, however, the "merge pull request" remains disabled due to the failure in one of the GitHub checks. That on windows.

Oh yes. Let me have another go at that tonight. Then, when you get a chance tomorrow, take a peak here and it might be ready to merge by you by then!!!

This was linked to issues Oct 21, 2024
@ernestguevarra
Copy link
Member

@tomaszaba all checks are now passing. I made some other changes as well. If you see this tomorrow, go ahead and merge to main. And then make a new branch for your next steps.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment