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

record upkeep year and use it for filtering checklist #2070

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

thomasp85
Copy link
Member

Fix #1916

This PR adds two new functions: record_upkeep_year() which writes the year of the upkeep to Config/usethis/upkeep and last_upkeep_year() which retrieves this value or defaults to 2000 if it doesn't exist. These two functions are used in use_tidy_upkeep_issue() to automatically use the year of the last upkeep for filtering the produced checklist as well as incrementing the recorded year to the current year.

Further, this PR fixes what appears to be a bug/discrepancy in how year was used in make_upkeep_issue() depending on whether use_upkeep_issue() or use_tidy_upkeep_issue() was called

@thomasp85 thomasp85 requested a review from jennybc October 8, 2024 07:24
Copy link
Member

@jennybc jennybc left a comment

Choose a reason for hiding this comment

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

My first / early reaction is that it feels like we have various year arguments lying around, some of which refer to the year of the upkeep and others which refer to the year of the previous upkeep. To be clear, this is not a situation of your creation -- it largely pre-existed.

Do you agree? Do you think this is a good occasion to make that situation less confusing?

Left a couple other comments.

#' }
use_upkeep_issue <- function(year = NULL) {
make_upkeep_issue(year = year, tidy = FALSE)
}

make_upkeep_issue <- function(year, tidy) {
make_upkeep_issue <- function(year, last_year, tidy) {
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if it's better to frame this as "previous (upkeep) year"? Seems less vulnerable to confusion about what "last year" actually means. So I'm proposing previous_year or prev_year.

Copy link
Member

Choose a reason for hiding this comment

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

Also, from #1916, do you think we should be incorporating this idea throughout?

Recording year and month is probably worthwhile.

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 wondered about this as well. On one hand we are currently not using month to anything (which is we I went the way I did), but on the other hand we may do upkeep biannually which means recording year is not enough.

I'm happy to add month as well and then we can always figure out if/how we want to use that information

@thomasp85
Copy link
Member Author

Yeah, I was a bit perplexed about the duality of year when I started digging into this. But I also came to the conclusion that I'd rather not break anything so I kept it as is and instead changed the code to actually do as advertised.

I kinda don't think an argument for "current year" is needed at all - I can't imagine a situation where it would be anything other than what can be extracted from Sys.Date(). So if we were to change things up I'd move towards deprecating that argument and perhaps change the name of year were it refers to last upkeep year to something more descriptive

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.

Record last upkeep year
2 participants