Demo of roxy tools

[dgkf]

@danielinteractive and I have been toying with the idea of building some supporting tools for linting documentation and we thought this package might serve as a good testing ground.

This PR is primarily as a demo. Happy to gather feedback and iterate if anything can be improved. Even if this is out-of-scope for savvyr, this PR can still serve as a discussion of using these tools in practice and closed without merging.

I'll add notes in-line below, but just want to call out a few features where I'd especially appreciate feedback:

• Added tags included @typed, @typedreturn (this feels too verbose to me)
• roxytypes has support for a @default "tag" (it's not really a roxygen2 tag, just part of the @typed parsing). I'm still debating whether there's a better way to provide this feature.
• roxylints defaults follow the tidyverse style guide, but openpharma deviates in a few places so those styles are customized in man/roxylints/meta.R.

[numbersman77]

Happy to update all this. I won't continue adding new functions until this PR has been merged.

[danielinteractive]

Thanks a lot @dgkf ! This is amazing!

[danielinteractive]

@dgkf If I understand @cicdguy information correctly from the recent ggpp experience, then you need to authorize github actions bot to run actions on your fork (note that you could also do a branch in this repo because I gave you developer rights)

[danielinteractive]

And here some thoughts on the concrete questions:

• @typedreturn could be replaced by @result or something similar? or we could not include it at all. For me the main thing is the arguments
• The @default thing I don't understand yet I think so cannot comment - but if it is a required implementation detail it would be ok from my side
• Yes tidyverse as default makes most sense - awesome that it can be customized, that really makes it versatile!
• For choosing whether to allow or disallow new rules, an alternative could be another list element, e.g. new_typed_rules = FALSE would disallow new rules.

For later (not now) it would be great if roxytypes has a function that makes migration from an existing @param based format to a @typed format easy - I am thinking especially about large existing packages there.

[dgkf]

Thanks for all the thoughtful review. Based on your feedback I opened up some designated threads for these specific topics.

@typedreturn could be replaced by @result or something similar? or we could not include it at all. For me the main thing is the arguments

Added this suggestion as well as a list of some other options I was thinking about: openpharma/roxytypes#4

The @default thing I don't understand yet I think so cannot comment - but if it is a required implementation detail it would be ok from my side

See the description of this behavior here: https://github.com/dgkf/roxytypes#defaults. I think it's a cool capability, but might not need to live in the roxytypes package. Discussion here: openpharma/roxytypes#5

For choosing whether to allow or disallow new rules, an alternative could be another list element, e.g. new_typed_rules = FALSE would disallow new rules.

Yeah, I think something like that would be better. I opened a proposal here: openpharma/roxylint#1

For later (not now) it would be great if roxytypes has a function that makes migration from an existing @param based format to a @typed format easy - I am thinking especially about large existing packages there.

I think this might be a quick win. Let me see if I can throw something together quickly, but if it ends up being more work I'll leave it for a later date. Proposed behaviors here: openpharma/roxytypes#3

[danielinteractive]

Thanks Doug - would you like to merge this PR in current form? or would you like to address all this first?
Just so that @numbersman77 and @kuenzelt know whether they should wait for this to be merged or not before continuing

[dgkf]

would you like to merge this PR in current form

I'll leave that up to you all. If you're comfortable working with it while it's still a bit in pre-release mode, that's totally fine by me as long as you're okay with things still evolving a bit.

If you want to lock the version to the current one to be safe, that's fine too. I'll continue chipping away at some of the features mentioned above.

[danielinteractive]

Cool. Then I would say let's get this merged here and we can be alpha testers

[dgkf]

CI issues sorted out. Only remaining issue is a failure because the actions bot is attempting to push to my fork which it doesn't have permission to do. I don't have permission to merge, so I think it's up to you all to decide when it's the right time to incorporate the changes.

[danielinteractive]

Thanks Doug! Can you maybe give the bot permission? And I thought I added you to developers so after checks pass you should be able to merge

[dgkf]

I would prefer not to. It's a bit of a weird request to ask that anyone who forks the repo provide permissions to it in order to contribute. I've never seen another project that works this way.

I'm not as familiar with gh actions, but I imagine this job could use a rule like this one so that it is only executed upon merge, avoiding any issues with permissions on forks.

@cicdguy - what do you think about moving the CD jobs to operate on merges instead of commits?

[danielinteractive]

@dgkf yeah no problem, actually just looking at the details now you can just restyle your files (with styler) and the checks should pass fine, so no need for the bot to push style updates on your fork.
(btw in the future please directly work on a branch in this repo, I had added you earlier to developers, then it is easier :-))

[dgkf]

btw in the future please directly work on a branch in this repo

Can do, but I think this was a good smoke test. I don't think a contributor workflow should require org membership. For most projects the pull-request-from-fork is really the norm.

[cicdguy]

FWIW, I've fixed the autocommit feature in the styler workflow to not attempt to push commits if the PR came from a fork, like in this case. This will warn the user that the files weren't styled instead of attempting to push to the contributor's branch. Users don't need to provide any additional permissions to bots to be able to push content to their repos, as this poses a security risk that the user should not assume.