Skip to content

Conversation

@maelle
Copy link
Contributor

@maelle maelle commented May 19, 2023

Coming from r-lib/desc#117 (comment)

I understand why this doesn't happen in usethis test where testthat edition field is already set by create_local_package(); after digging a bit I see that prior to 747931b there was as.character(). Now I am at peace, having the whole story 🧩 😌

@krlmlr

@jennybc
Copy link
Member

jennybc commented May 19, 2023

@ateucher will you have a look? I think you were most recently in this headspace. But seems good to me.

@ateucher
Copy link
Collaborator

Yes, happy to.

Copy link
Collaborator

@ateucher ateucher left a comment

Choose a reason for hiding this comment

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

Thanks @maelle this is great. Along with this change I think it's also worth making proj_desc_field_update() stricter here so this would be handled there rather than in desc (probably using check_string(value)).

Happy to make that change myself if you'd prefer.

@jennybc
Copy link
Member

jennybc commented May 23, 2023

Should proj_desc_field_update() be in charge of coercing to character?

@ateucher
Copy link
Collaborator

I did certainly wonder about that but thought it may end up receiving something unexpected that goes awry when trying to coerce, whereas the calling functions would be more suited to formatting the value properly since they know about them. Right now use_testthat_impl() is the only function that calls it with a non-character value.

@jennybc
Copy link
Member

jennybc commented May 23, 2023

OK I'm happy for you decide. So, yes, I think it would be good to check for character input, as you suggest above.

* Also allow vectors > length 1 since desc() allows it
* union old and new when appending to avoid duplicates
@ateucher
Copy link
Collaborator

I'm going to merge this - thanks @maelle!

@ateucher ateucher merged commit 418bc89 into r-lib:main May 23, 2023
Code
proj_desc_field_update("Config/Needs/foofy", c("alfa", "bravo"), append = TRUE)
Message
v Adding 'alfa', 'bravo' to Config/Needs/foofy
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure this would be worth the fiddliness. But arguably the set logic should be applied before we message. Maybe just worth keeping in mind if we touch this again.

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.

4 participants