vincentarelbundock / countrycode

R package: Convert country names and country codes. Assigns region descriptors.
https://vincentarelbundock.github.io/countrycode
GNU General Public License v3.0
342 stars 83 forks source link

Enable global roxygen2 Markdown support #268

Closed salim-b closed 3 years ago

salim-b commented 3 years ago

and tidy DESCRIPTION file (using usethis::use_tidy_description()).

Markdown syntax is already used at various places in the documentation but wasn't rendered correctly since the roxygen2's Markdown support wasn't turned on.

vincentarelbundock commented 3 years ago

Sounds like a great improvement, thanks so much for looking into this!

But it looks like you only modified the .Rd files. Won't those get overwritten when we run roxygen again?

salim-b commented 3 years ago

But it looks like you only modified the .Rd files. Won't those get overwritten when we run roxygen again?

Sure, they'll get overwritten when roxygenizing the next time – but from now on with global Markdown support :wink: The relevant change is just this line:

https://github.com/vincentarelbundock/countrycode/blob/646b7bb80ab860225a58fa89009dcc3043d72e4e/DESCRIPTION#L37

vincentarelbundock commented 3 years ago

Ah, got it! Thanks, this is great!

cjyetman commented 3 years ago

@salim-b is there a good reason to target a development version of roxygen2 instead of the release version?

cjyetman commented 3 years ago

also, any ideas why this doesn't pass tests on Ubuntu 3.3, 3.4, and 3.5?

each fail with...

Error in gsub("\r\n", "\n", str, fixed = TRUE) : input string 1 is invalid in this locale Calls: -> new_rcmdcheck -> win2unix -> gsub Execution halted Error: Process completed with exit code 1.

which seems strange, but this wasn't happening before as far as I can tell.

vincentarelbundock commented 3 years ago

Lesson to me: Never merge late at night ;)

I have no idea what this error code means, but maybe this PR can eventually fix it https://github.com/vincentarelbundock/countrycode/pull/269

I'm not sure we are "targeting" a particular roxygen2 release. IIRC, the note just writes what version was used; it's not a requirement.

cjyetman commented 3 years ago

I thought the RoxygenNote: 7.1.1.9001 in the DESCRIPTION file was specifying that you want to use specifically that version of roxygen to generate docs... but I may be misinterpreting that. I didn't think that roxygen modified the DESCRIPTION file like that though.

vincentarelbundock commented 3 years ago

Right, it appears this was a conscious design decision: https://github.com/r-lib/roxygen2/issues/905

cjyetman commented 3 years ago

interesting... I'm not a fan of that behavior, but I guess it's serving its purpose here. So when we see a change like RoxygenNote: 7.1.1.9001, we should be asking ourselves "was this generated with the 'proper' version of roxygen", whatever is determined to be "proper".

salim-b commented 3 years ago

@salim-b is there a good reason to target a development version of roxygen2 instead of the release version?

As @vincentarelbundock already wrote, this is roxygen2's standard behavior. It wasn't really intentional from me, I just happen to use the development version of roxygen2 for now until the next stable release (IIRC there was something fixed that was bothering me since the last stable release). I've inspected the changes to the .rd files before committing and everything seemed fine, so it really shouldn't be an issue.

cjyetman commented 3 years ago

I'd say it is an issue if it's something that gets changed every time someone does an update to the documentation. There's a cost in having reviewers have to figure out whether or not a change is necessary/good/dangerous/critical/etc.

vincentarelbundock commented 3 years ago

Agreed, but this seems like handling version specific consequences of toolchains is something that falls within the purview of tools like renv or docker. Unfortunately, there's not much we can do here, since it's quite likely that maintainers will have non-matching versions of roxygen2 on their computers at some points.