Review of {admiraldev} Developer Guide pages ahead of 1.0

[manciniedoardo]

Please share your thoughts about what should be enhanced/modified/removed ahead of 1.0.

[manciniedoardo]

Development Process page - list your comments below.

[manciniedoardo]

Programming Strategy page - list your comments below.

[zdz2101]

I made a draft PR to make the easier to see in context/ the side-by-side https://github.com/pharmaverse/admiraldev/pull/343/files

Highlights would be:

• I don't think we actually comment as well as we describe in our programming strategy
• some of our keyword/family feel a little outdated/do we ever want to rearrange the categories?
• Sometimes there's R-in-general type of things not necessarily tied to admiral like the conversion of blanks/NAs from SAS, think we would benefit from removal and pointing people to the fantastic textbooks that achieve this
• file structuring section doesn't feel aligned with what we actually do
• I don't know if we technically do the whole suppress_warning portion of step 1 of deprecation as described

[bms63]

I made a draft PR to make the easier to see in context/ the side-by-side https://github.com/pharmaverse/admiraldev/pull/343/files

Highlights would be:

• I don't think we actually comment as well as we describe in our programming strategy

Well...more of a goal to reach :)

• some of our keyword/family feel a little outdated/do we ever want to rearrange the categories?

Yes happy to re-visit. I think since we are more "stable" that a revisit makes a lot of sense.

• Sometimes there's R-in-general type of things not necessarily tied to admiral like the conversion of blanks/NAs from SAS, think we would benefit from removal and pointing people to the fantastic textbooks that achieve this

Not sure I am following.

• file structuring section doesn't feel aligned with what we actually do

Yeah - I think we should re-visit this in a meeting.

• I don't know if we technically do the whole suppress_warning portion of step 1 of deprecation as described

Shall we update to our current practice?

[manciniedoardo]

Functional Workflow:

• Admiral should be corrected to admiral

Functions in R:

• admiraldev should be corrected to admiraldev

Deprecation:

• Rename to deprecation/supercession (is that a word?) and outline our strategy for superseding functions

[bundfussr]

Is this still true? Seems like we allow global objects now??

I would consider this still as true. At the moment we use the objects from the admiral_environment as default values only. So they are still inputs to the function.

[bundfussr]

Unless specified by the user?

I see not need to change. I am not aware of admiral functions which return the dataset with a specific sort order.

[manciniedoardo]

Writing Vignettes page - list your comments below.

[bms63]

Just trying to cut down on the amount of reading a new dev needs to do. We don't really have anything admiral-flavored here so was thinking a new dev would just need to read the r-pkg manual to become familiar with vignettes

[manciniedoardo]

I would argue that portions such as this one could be quite useful for, say, an extension package team just starting out. Although, granted, you could just look at an existing vignette and copy that...

[bms63]

I think it could just go into the Programming Strategy as a section.

[manciniedoardo]

Can do, although we will be making the programming strategy quite long!

[bms63]

Going to leave as is

[manciniedoardo]

Unit Test Guidance page - list your comments below.

[bms63]

We have some interesting flavors with our Unit Tests, but I don't think it needs its own vignette. Can we move relveant admiral flavors into Programming Strategy? There is already a section for it?

[manciniedoardo]

I agree with this. In particular I think the first two sections ("Why Write Unit Tests" and "Writing Good Unit Tests") are not specific to {admiral} so they should be removed. In fact, from the third section I would only retain information about the add-in.

[bms63]

Well then we will have room for some more vignettes that I know @ddsjoberg wants to write!!

[bms63]

Going to keep and move the Programming Strategy testing stuff into here

[manciniedoardo]

Guidance for git and github usage page - list your comments below.

[manciniedoardo]

The "Introduction" and "Branches" and "Working with Feature Branches" are out of date. Now that we are working with the githubflow strategy maybe we can just reference to that in the "Contributing to {admiral}" page on the main website and do away with this page entirely.

[bundfussr]

The vignette needs to be updated but I would not remove it. I think the naming conventions for branches and commit messages are important and we need to add something regarding version numbering and update of DESCRIPTION (Remotes: field and using .9000 in required version).

[manciniedoardo]

PR Review guidance page - list your comments below.

[manciniedoardo]

Generally I think we should retain this page, with the modifications below:

• We should remove references to devel here
• Link to "Development Process" page will have to be updated if we do end up moving that page
• The "GitHub Actions/CI Workflows" section can be shortened and we can just link to the admiralci website (which should be updated ahead of 1.0)

[manciniedoardo]

R CMD Issues page - list your comments below.

[manciniedoardo]

I think the contents of this page should be updated and moved to the admiralci website

[manciniedoardo]

Release strategy page - list your comments below.

[manciniedoardo]

I think this page could be superseded by linking to a page exemplifying the githubflow release strategy (if there is one?)

[zdz2101]

https://www.atlassian.com/git/tutorials/comparing-workflows/gitflow-workflow#:~:text=The%20overall%20flow%20of%20Gitflow%20is%3A%201%201.,is%20merged%20into%20develop%20and%20main%20More%20items

Atlassian is certainly a leader in the space, going to expect a lot of organizations are using their enterprise software suite so probably used to reading their documentation too

[bundfussr]

I think the steps for normal releases and hotfix releases are quite useful and it is unlikely that we find an external page with this information. An update is required due to the change of the branching strategy (see pharmaverse/admiraldev#330 for a draft).

[manciniedoardo]

This page was recently updated, I see no reason for changes here :)

[manciniedoardo]

Package Extensions page - list your comments below.

[manciniedoardo]

This page was recently updated, I see no reason for changes here either

[bms63]

Admiral Manifesto Revisited

[bms63]

I think this should be made into a infographic on admiral and the text should live on the readme in admiraldev. The infographic should link to the admiraldev readme.

[manciniedoardo]

Agree with this suggestion.

[rossfarrugia]

Good discussions team! Adding some high level thoughts in case useful here @bms63 and @manciniedoardo - overall i think a lot of work could potentially be created here updating these pages but i would focus on value updates here (e.g. where our dev process has changed like git branching strategy) rather than an overhaul given that i expect we shouldn't need as many new community contributors compared to when we were first building up this package. also there was a few mention of removing some pages that gave more generic guidance such as unit testing - i get that as they're not related specifically to admiral and maybe one day we'd move these to some higher level pharmaverse location, but fyi i have been sharing these with other pharmaverse pkg dev teams like datacutr and sdtmchecks. people have found them really useful, so i'd be in favour of not removing any content for now and down the line if we find a better place to store it then we could move. p.s. tag me in any reply if you'd like my further input, as i won't ask for notifications on this thread for fear of an already busy inbox exploding 😄

[bms63]

Okay sounds good on keeping the vignettes. I was just trying to cut down on paperwork that is already repeated in the r-pkg manual.

Understood on not trying to do a serious overhaul. I am mostly interested in removing then re-writing and making sure there is nothing contradictory in our strategy.

[bms63]

Revamp of admiraltemplate

[bundfussr]

Do we need admiraltemplate? Do we expect more admiral extension packages?

[bms63]

I think it is worthwhile to keep in action. I know a few other non-admiral projects made use of the template. It will need a bit of touch up post-1.0 and renv removal.

[bms63]

but for other extensions - that is the dream, but I don't know of any in the queue.

[manciniedoardo]

Agree we should keep - we have no extension packages currently in the works but this could change at any moment if the nee /opportunity arises or an interested individual/company comes forward.