MichaelChirico / potools

Tools for working with translations in R
https://michaelchirico.github.io/potools/
58 stars 3 forks source link

Experiment implementing po_extract() #243

Closed hadley closed 2 years ago

hadley commented 2 years ago

Mostly to trigger discussion. More comments below.

MichaelChirico commented 2 years ago

po_scan() doesn't feel like a great name to me... maybe po_snapshot()?

hadley commented 2 years ago

Hmmm, it seems like we're cueing off different parts of this function — it both looks for (scans) and records (snapshots) messages for translation. I wonder if we can somehow get both ideas into one word ... How about po_extract()?

hadley commented 2 years ago

Fixes #223. Fixes #226. Fixes #229.

MichaelChirico commented 2 years ago

po_extract() sounds better to me -- both make clearer the output of the function... the scanning part feels more "incidental"

hadley commented 2 years ago

Ok, I think this is pretty close to being done apart from the documentation — the problem is that po_extract() andget_message_data()share all of their arguments but there's no way to@inheritParams` from an .Rd file in the same package. I think there are two options:

  1. Convert get_message_data() to use roxygen2
  2. Duplicate the docs

Which would you prefer?

MichaelChirico commented 2 years ago
  1. I'm happy to gradually converge on roxygen2 everywhere
hadley commented 2 years ago

@MichaelChirico if you want, I can do a separate PR that converts all the .Rds to roxygen (it's 95% automated). Do you want to use @export too or stick with the explicit NAMESPACE?

MichaelChirico commented 2 years ago

That works, esp. if it'll make it easier for you to be productive going forward (and assuming it's not a huge time drain for you).

I would keep an explicit NAMESPACE, I do prefer being more intentional there.

hadley commented 2 years ago

To be clear, I meant using @export to generate the NAMESPACE, so it's still intentional, but you while you're near the source of the function you can tell if it's exported or not.

MichaelChirico commented 2 years ago

right -- I like that but the @import / @importFrom less so.

how about using the NAMESPACE roclet but keeping all the @import/@importFrom tags in a central place (e.g. the @doctype package or .onLoad)?

hadley commented 2 years ago

Sure. Our current convention is to put them with the package docs anyway.