Fix idempotency of roxygenize() for packages using multi-line @rawNamespace

[MichaelChirico]

Closes #1572. NB: This includes #1571, which I used to write the update. I can excise it but leaving it in for now.

Tests still needed. Filing for discussion first on if this is the right approach.

There are two parts to the fix:

1. Collapse the parsed entries to one line in namespace_exports(). This matches the output of namespace_imports().
2. Except for namespace_imports() reads in a leading \n, so we use trimws() too.

---

That's all well and good for the example we have in mind for #1572, but I do wonder if the issue is broader. That the @rawNamespace entry trips up namespace_exports() has a smell to it -- the @rawNamespace code is only (nested) import directives, why is it being pulled in by namespace_exports()? OTOH we probably don't want to traverse the AST for these entries, which can in principle break the dichotomy of import vs. export directives (nothing prevents us from putting an export() entry in one branch and an import() entry in the other, or a mix of the two types in either/both branches). So maybe this is just a case of namespace_exports() being misleadingly named?

[MichaelChirico]

Have a working test but it feels clunky, you might suggest something snappier :)

(nvm, it doesn't WAI because update_namespace_imports() simply skips because we lack the "made by roxygen2" note, so this test also passes on main -- open to ideas here)

[hadley]

IIRC the idea is that we can't easily tell if @rawNamespace is an import or an export, we should treat it as if it is. If it's not actually an import directive, it probably won't cause a problem.

But yeah, namespace_exports() is really namespace_not_imports() so that union(namespace_imports(), namespace_exports()) gives you the complete namespace.

Your suggested fix makes sense to me.

[hadley]

Can you please merge/rebase and add a news bullet?

[MichaelChirico]

Can you please merge/rebase and add a news bullet?

Done -- still not sure the right approach for testing (as noted, the current test is no good because it actually passes on current main, i.e., independently of the change here).

One option is to add another test package, I just wonder if you see a way to get the regression test without needing to add a whole fake package.

[hadley]

What if we just test the assertion that namespace_exports() returns one line for each expression?

[MichaelChirico]

Hmm I worry that tests an implementation detail rather than being a regression test for the specific bug. For example, such a test could in the future keep working while due to refactor multi-line @rawNamespace as in the motivating example breaks again.

Added a minimal test package instead, and confirmed it fails on current main.

[hadley]

@MichaelChirico do you think it's worth a patch release for this?

[MichaelChirico]

@MichaelChirico do you think it's worth a patch release for this?

I see 6 up votes on #1570, so between these two and probably #1575, I would say yes.

[hadley]

Ok, I'll add to my to do list.