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

docs: remove explicit cookbook section numbering #262

Merged
merged 8 commits into from
Dec 2, 2024

Conversation

jdhoffa
Copy link
Member

@jdhoffa jdhoffa commented Nov 22, 2024

The order is defined implicitly by the order in which it appears on the pkgdown.yml:

contents:
- cookbook_overview
- cookbook_preparatory_steps
- cookbook_running_the_analysis
- cookbook_interpretation
- cookbook_advanced_use_cases

Explicit numbering of the sections is superfluous and clunky.
Screenshot 2024-11-22 at 13 02 13

@jdhoffa jdhoffa requested a review from jacobvjk as a code owner November 22, 2024 12:03
Copy link

codecov bot commented Nov 22, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 51.61%. Comparing base (3f08f0d) to head (6bda58b).
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #262   +/-   ##
=======================================
  Coverage   51.61%   51.61%           
=======================================
  Files          29       29           
  Lines        3189     3189           
=======================================
  Hits         1646     1646           
  Misses       1543     1543           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.


🚨 Try these New Features:

@cjyetman
Copy link
Member

I wonder if they even need the "Cookbook - " prefix? Somewhat clear from the section header?

@jdhoffa
Copy link
Member Author

jdhoffa commented Nov 22, 2024

I wonder if they even need the "Cookbook - " prefix? Somewhat clear from the section header?

See the code, I had removed that too haha

@jacobvjk
Copy link
Member

If we do this, I think it would be good to add an overview at least to the first chapter that describes what is to come, just for user orientation. While I agree that it is implicit from the dropdown in the nav bar, that is not exactly a typical way to present the contents one can expect from a document.. I think the target audience is not necessarily one that hangs out on GH a lot, so I think being a bit more explicit somewhere in the cook book does not hurt

Also, the links at the bottom of the chapters should be renamed as well then

@jdhoffa
Copy link
Member Author

jdhoffa commented Nov 22, 2024

That makes sense. Then I will adjust.

@jdhoffa jdhoffa marked this pull request as draft November 22, 2024 12:50
@jacobvjk
Copy link
Member

bit unsure why the macos check keeps failing, the same failure has recently been popping up elsewhere, but re-triggering it has usually solved it.. guess we may just have to wait and see, since it seems to be related to the latest R version

@cjyetman
Copy link
Member

bit unsure why the macos check keeps failing, the same failure has recently been popping up elsewhere, but re-triggering it has usually solved it.. guess we may just have to wait and see, since it seems to be related to the latest R version

definitely something widespread going on:
r-lib/actions#950
r-lib/actions#948

Looks like either an intermittent GH Actions network problem, or something to do with the latest GH Actions macOS runners being arm64 and not x86 and possibly not building R or its dependencies properly.

@cjyetman
Copy link
Member

likely something with the macos-latest GH Action runner
r-lib/actions#948 (comment)

supposedly a new version (20241022.361) is currently rolling out, last run here failed with the old version (20241119.509) at 2024.11.23 17:42 GMT+1
https://github.com/RMI-PACTA/pacta.multi.loanbook/actions/runs/11973244713/job/33424388196#step:1:9

more detail likely related: actions/runner-images#10864 (comment)

@cjyetman
Copy link
Member

macOS-latest failure resolved by GitHub's latest macOS 14 runner image 20241125.556
https://github.com/actions/runner-images/releases/tag/macos-14-arm64%2F20241125.556

@jacobvjk
Copy link
Member

jacobvjk commented Dec 2, 2024

That makes sense. Then I will adjust.

I took the liberty to address this, so that we can close this PR soon @jdhoffa

@jdhoffa
Copy link
Member Author

jdhoffa commented Dec 2, 2024

Thank you!! I will leave the rest of this PR to you then.
Feel free to open it from Draft whenever you're ready

@jacobvjk jacobvjk marked this pull request as ready for review December 2, 2024 10:51
@jacobvjk jacobvjk requested a review from cjyetman December 2, 2024 10:51
Copy link
Member

@cjyetman cjyetman left a comment

Choose a reason for hiding this comment

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

looks good, just one nit comment about making the section descriptions linkable

vignettes/cookbook_overview.Rmd Outdated Show resolved Hide resolved
@jacobvjk jacobvjk requested a review from cjyetman December 2, 2024 11:03
@jacobvjk jacobvjk merged commit 7068b6f into main Dec 2, 2024
10 checks passed
@jacobvjk jacobvjk deleted the remove_explicit_cookbook_indexes branch December 2, 2024 11:17
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