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

feat: bump_version() only works in interactive sessions or if NEWS.md has a preamble (or both) #638

Merged
merged 21 commits into from
Apr 12, 2023
Merged

feat: bump_version() only works in interactive sessions or if NEWS.md has a preamble (or both) #638

merged 21 commits into from
Apr 12, 2023

Conversation

maelle
Copy link
Member

@maelle maelle commented Feb 27, 2023

Fix #596

@github-actions github-actions bot mentioned this pull request Feb 27, 2023
Closed
maelle
maelle commented Feb 27, 2023
View reviewed changes
R/api-bump-version.R Outdated Show resolved Hide resolved
maelle
maelle commented Feb 27, 2023
View reviewed changes
R/fledgling.R Outdated
@@ -177,8 +177,9 @@ read_fledgling <- function() {
date <- read_date()

news_and_preamble <- read_news()
news_and_preamble[["preamble"]] <- news_and_preamble[["preamble"]] %||% news_preamble()
Copy link
Member Author

Choose a reason for hiding this comment

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

I moved the preamble "filling" here in order to be able to use read_news() for checking whether there's a preamble in the NEWS.md file.

maelle
maelle commented Feb 27, 2023
View reviewed changes
R/repo-helper.R Show resolved Hide resolved
maelle
maelle commented Feb 27, 2023
View reviewed changes
R/utils.R Outdated Show resolved Hide resolved
@maelle maelle requested a review from krlmlr February 27, 2023 13:40
krlmlr
krlmlr reviewed Feb 27, 2023
View reviewed changes
R/utils.R Outdated Show resolved Hide resolved
R/utils.R Outdated Show resolved Hide resolved
R/api-bump-version.R Outdated Show resolved Hide resolved
@maelle maelle changed the title move preamble filling feat: bump_version() to do nothing if non-interactive and no preamble in NEWS.md Feb 27, 2023
krlmlr
krlmlr reviewed Feb 27, 2023
View reviewed changes
Copy link
Contributor

@krlmlr krlmlr left a comment

Choose a reason for hiding this comment

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

Thanks. Do we even need preamble_in_file ? We can leave it for now, but if it doesn't prove super necessary we can also remove it in the next iteration.

R/api-bump-version.R Outdated Show resolved Hide resolved
maelle
maelle commented Mar 20, 2023
View reviewed changes
R/api-update-news.R Outdated Show resolved Hide resolved
@maelle maelle requested a review from krlmlr March 20, 2023 08:51
krlmlr
krlmlr requested changes Mar 23, 2023
View reviewed changes
Copy link
Contributor

@krlmlr krlmlr left a comment

Choose a reason for hiding this comment

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

Thanks. Let's keep the API unchanged for now.

R/api-update-news.R Outdated Show resolved Hide resolved
R/bump-version.R Outdated Show resolved Hide resolved
R/fledgling.R Outdated Show resolved Hide resolved
R/repo-helper.R Show resolved Hide resolved
R/update-news.R
@@ -1,6 +1,6 @@
# File editing ------

update_news_impl <- function(commits, which) {
update_news_impl <- function(commits, which, fledgeling = NULL) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Would the following be an anti-pattern, violating https://design.tidyverse.org/def-short.html and similar?

Suggested change
update_news_impl <- function(commits, which, fledgeling = NULL) {
update_news_impl <- function(commits, which, fledgeling = read_fledgeling()) {

Copy link
Member Author

Choose a reason for hiding this comment

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

I think using NULL is the right choice?

The most common approach is to use NULL as a sentinel value that indicates that the argument is optional, but non-trivial.

@github-actions github-actions bot mentioned this pull request Apr 11, 2023
Closed
@maelle maelle requested a review from krlmlr April 11, 2023 15:21
@krlmlr krlmlr changed the title feat: bump_version() to do nothing if non-interactive and no preamble in NEWS.md feat: bump_version() only works in interactive sessions or if NEWS.md has a preamble (or both) Apr 12, 2023
@krlmlr krlmlr merged commit b421db8 into main Apr 12, 2023
@krlmlr krlmlr deleted the preamble-showstopper branch April 12, 2023 07:15
@krlmlr
Copy link
Contributor

krlmlr commented Apr 12, 2023

Thanks!

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 12, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@krlmlr krlmlr Awaiting requested review from krlmlr

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Watch out for "generated by" header
2 participants
@maelle @krlmlr