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.

Sign up for GitHub

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

Jump to bottom

Fixed version of #1346: Allow split_by to accept numeric values #1490

Merged
merged 18 commits into from
Apr 4, 2025
Merged

Fixed version of #1346: Allow split_by to accept numeric values #1490

merged 18 commits into from
Apr 4, 2025

Conversation

katrinabrock
Copy link
Contributor

@katrinabrock katrinabrock commented Feb 6, 2025 •
edited by yihui
Loading

Close #1346

@katrinabrock katrinabrock changed the title Split by rebased Fixed version of #1346: Allow split_by to accept numeric values Feb 6, 2025
@katrinabrock
Copy link
Contributor Author

Same content as #1346 but with the failing tests fixed :-)

@katrinabrock katrinabrock mentioned this pull request Feb 6, 2025
Closed
@katrinabrock
Copy link
Contributor Author

You can cleanly cherry pick every thing after the add tests... commit on to @lcougnaud 's PR if that's better for giving them proper credit than merging this one.

@katrinabrock
Copy link
Contributor Author

@yihui The code in this one is ready for review btw. Not sure if that was clear from my above comments.

The commit messages certainly leave something to be desired. I can fix them and make sure @lcougnaud is show as the author of the code they wrote if that matters. Let me know. I figured those messages would get squashed away anyhow.

@lcougnaud
Copy link
Contributor

@katrinabrock thanks for fixing the tests & finalizing the changes! For me it is all fine to squash everything into one single commit during the merging.

yihui
yihui reviewed Feb 19, 2025
View reviewed changes
Copy link
Member

@yihui yihui left a comment

Choose a reason for hiding this comment

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

To be honest, I feel this is beyond my capability to review, and I'll simply trust your hard work. Before I merge it, could you test a couple of books and see if they render to the same HTML files before/after this PR? For example:

You can first render a book without this PR, temporarily commit the HTML output files in Git, and then install bookdown with this PR (and restart R), render again. Then see if there are any changes in Git.

Thanks!

@yihui yihui linked an issue Feb 19, 2025 that may be closed by this pull request
[FR] Support split_by for section level higher than 2 (section) in gitbook #1347
Closed
@katrinabrock
Copy link
Contributor Author

ok! yes, I can do that. I will look also take closer at the code written by @lcougnaud before you blindly merge so at least there are two sets of eyes on it.

@yihui
Copy link
Member

yihui commented Feb 20, 2025

Great! If you run into any errors when building these books, you can add eval = FALSE to the code chunks that produced the errors (or even delete them).

katrinabrock and others added 6 commits March 31, 2025 11:21
@katrinabrock
lcougnaud's split_by squashed
59c3671 
Squashed commit of the following:

commit 99f8698
Author: Laure Cougnaud <laure.cougnaud-ext@glpg.com>
Date:   Tue Jun 14 10:33:16 2022 +0000

    limit numeric split_by to: [1:6] + update documentation

commit 3304d5a
Merge: 7f1e38b 4cdf9c6
Author: Laure Cougnaud <laure.cougnaud-ext@glpg.com>
Date:   Tue Jun 14 08:20:17 2022 +0000

    merge branches

    Merge remote-tracking branch 'OAGithubRStudio/main' into split_by-numeric

    # Conflicts:
    #	R/gitbook.R
    #	R/html.R
    #	man/build_chapter.Rd
    #	man/epub_book.Rd
    #	man/gitbook.Rd
    #	man/html_chapters.Rd
    #	man/html_document2.Rd
    #	man/pdf_book.Rd
    #	man/publish_book.Rd
    #	man/render_book.Rd
    #	man/resolve_refs_html.Rd
    #	man/serve_book.Rd

commit 7f1e38b
Author: lcougnaud <laure.cougnaud@openanalytics.eu>
Date:   Mon Nov 4 11:03:18 2019 +0100

    fix for nested subsection (part 1)

commit ee342c7
Author: lcougnaud <laure.cougnaud@openanalytics.eu>
Date:   Thu Oct 31 16:17:33 2019 +0100

    fix specification split_by

commit 60bb103
Author: lcougnaud <laure.cougnaud@openanalytics.eu>
Date:   Thu Oct 31 14:25:09 2019 +0100

    fix inclusion subsections

commit 9c11691
Author: lcougnaud <laure.cougnaud@openanalytics.eu>
Date:   Thu Oct 24 18:05:31 2019 +0200

    split_chapters: fix numbering higher level sections that split_by

commit 0442817
Author: lcougnaud <laure.cougnaud@openanalytics.eu>
Date:   Thu Oct 24 17:42:57 2019 +0200

    update split_chapters for numeric 'split_by'
@lcougnaud @katrinabrock
add tests for split_by as numeric + improve indentation
c2a8ae8
@katrinabrock
order git book split_by
13a6dbf
@katrinabrock
Fix rmd test failure
cf83ce1
@katrinabrock
Fix docs
1180d68
@katrinabrock
fix html_chapters defaults as well
cbdd3b3
@katrinabrock
Copy link
Contributor Author

katrinabrock commented Mar 31, 2025 •
edited
Loading

ok, I may have done a little overkill on the testing part. I set up this repo to test bookdown branches: https://github.com/katrinabrock/bookdown-heavy-testing . Hopefully this will be useful to you in testing other PRs as well.

Running this branch against the three books there are only a couple lines different. Diff Here The differences are coming from these two lines in the bookdown documentation:

Which is expected because those are the functions we're changing.

I still want to do one more pass looking closely over the code before merging to make sure I understand everything since it is not getting a full review.

tl;dr - all good as far as extended testing ✅ 💪, but please don't merge quite yet. ❌

@katrinabrock katrinabrock mentioned this pull request Apr 3, 2025
Closed
katrinabrock
katrinabrock commented Apr 3, 2025
View reviewed changes
@katrinabrock katrinabrock marked this pull request as draft April 3, 2025 21:58
@katrinabrock
Copy link
Contributor Author

@yihui This one is also ready.

I included the HTML validation tests from #1495 but not the CI part (plus more specific to this PR). So those are just being skipped for now. They are passing locally though.

You can merge this PR and #1495 in either order. Just whichever you merge first, I will resolve the conflicts with the other one.

Demonstration this is not does not change the output from the three books above is here: katrinabrock/bookdown-heavy-testing#1 I added the one expected diff to the exceptions file on that branch so it runs green.

@katrinabrock katrinabrock marked this pull request as ready for review April 4, 2025 19:22
yihui
yihui approved these changes Apr 4, 2025
View reviewed changes
Copy link
Member

@yihui yihui left a comment

Choose a reason for hiding this comment

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

Thanks again! This is incredible!

@yihui yihui merged commit bdcce30 into rstudio:main Apr 4, 2025
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yihui yihui yihui approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[FR] Support split_by for section level higher than 2 (section) in gitbook
3 participants
@katrinabrock @lcougnaud @yihui