Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Guidelines around lifecycle::badge in teal packages #86

Closed
donyunardi opened this issue Nov 14, 2024 · 9 comments
Closed

Guidelines around lifecycle::badge in teal packages #86

donyunardi opened this issue Nov 14, 2024 · 9 comments
Labels

Comments

@donyunardi
Copy link
Collaborator

donyunardi commented Nov 14, 2024

Summary

We used lifecycle::badge to indicate the stage of a function (e.g., experimental, stable, deprecated, superseded).

I found a past PR related to this topic:
insightsengineering/teal#456

However, we currently lack guidance on how these badges should transition from one stage to the next (i.e. from experimental to stable) or on when a badge should be removed (i.e. teal::init still has stable badge. Should it be removed?).

Acceptance Criteria:

  1. Discuss and decide on guidelines for transitioning or removing lifecycle::badge in teal packages.
  2. Implement changes based on the decision before the release.
@donyunardi
Copy link
Collaborator Author

donyunardi commented Nov 15, 2024

Great points from @llrs-roche

Perhaps as part of the insightsengineering/nestdevs-tasks#86 we could decide a policy on the life cycle of functions and arguments? It could incorporate how we search and find internal (looking at internal Roche's code repositories?, asking someone?) and external repositories (searching github?, asking other programmers from other companies?) that could be affected. Similar to how the tidyverse sends PR to affected packages before sending their new version to CRAN.

We'll add this topic to the discussion.

@llrs-roche
Copy link

teal framework adheres to many tidyverse ideas, such as style and lifecycle badges. These have 4 stages thought to be used like this:

image

Following the article I discuss how to apply the principles and ideas to the teal framework (quotes are from the stages article):

Stable

Because this [stable] stage is the default, functions will only be given a stable badge if there’s some specific need to draw attention to their status.

I suggest to only keep the stable badge for functions (or arguments) that have left the experimental phase, and keep them for 1 year or 1 release (which ever comes earlier).

Deprecated

A deprecated function has a better alternative available and is scheduled for removal. When you call a deprecated function, you get a warning telling you what to use instead.

As suggested this should only apply to functions or arguments that have replacement. This badge is used after the code has been already exposed to the user so I would keep the badge for 2 years or 2 release whichever comes later. While the project might be stuck with a given release for the duration of the project teal app developers might need time to adjust to the replacement function/argument.

The first year the function or argument could be soft deprecated and at the second year it could be hard deprecated and on the third year or third release to completely remove the function.

Experimental

This is the most ambiguous badge. For functions, I would only add it to those that provide a new functionality or are intended to replace existing ones, including helper functions.

For arguments, I would only add it to those arguments on existing functions that provide new functionality, not renaming arguments to be more consistent, or to each argument of new (experimental) functions.

An experimental function would mean all the arguments are experimental, a stable function would mean only new arguments are experimental.

As the fate of an experimental functions/arguments can quickly become deprecated/removed or stable, I wouldn't limit the time to a fixed term, but I would at least leave it for 1 release or 1 year which ever comes later, to gauge feedback from users (and developers). To move to stable I would require no negative feedback is received in the last year, few changes are made in the last year and we are happy with the development.
Remember that per the graph above, experimental functions or arguments do not change the status to deprecated or superseded.

Superseded

This badge is intended only for functions that won't be removed or updated (not even arguments or behavior), once added I wouldn't remove it. It also means the function is stable and shouldn't be having breaking changes or changes of arguments.

I don't think it is a good idea to promise user that a function won't be removed as this can lead to great work later on to keep the same bugs/behavior. I would only use it for very core functions (teal::init), but once added, the badge cannot be removed as it would lead to confusion to users.

Other considerations

All these changes and badges are mostly considering only internal factors, the only badge affected by users feedback is the experimental badge. It is then relevant to consider how a experimental function/argument is used.
To decide whether remove the experimental function, keep it experimental or move it to stable I would:

  1. check CRAN packages that might use the experimental functionality.

    Perhaps using some search like lang:R org:cran teal. If the function or argument is used by other packages evaluate how important it is. This is the most expensive task as it requires coordinated release of packages or breaking users code (if not version frozen) on new and different ways.

  2. Search on public Github

    Perhaps using some search like lang:R teal

  3. Search on internal search on code.roche.org

    Perhaps with something like: Language = R

  4. Ask via pharmaverse slack for feedback/usage

    This will reach developers both internal and external that we are considering changing the behavior of a function or argument and might provide additional feedback to check the direction of the change. I would leave a week to give them time to read the message and react.

With what is gathered consider the impact of the changes or if something needs to be discussed with the product owner or other stakeholders.


It might make sense to follow some approaches too from their design principles, this section about remediating past mistakes is relevant to lifecycle.

@shajoezhu
Copy link
Collaborator

from experimental to Superseded as well?

@llrs-roche
Copy link

@shajoezhu I'm not sure I follow you, but experimental arguments/functions shouldn't go to superseded according the the lifecycle and tidyverse team. Note also that this is my opinion and I've not worked on a clinical trial project using teal, my experience/opinions come from packages that are updated more frequently by users.

@shajoezhu
Copy link
Collaborator

I see. Thanks @llrs-roche , please ignore my comment

@donyunardi
Copy link
Collaborator Author

donyunardi commented Jan 8, 2025

Let's only focus on three badges for both function and argument:

  • experimental
  • stable
  • deprecated

The explanation that @llrs-roche makes sense to me (Thanks @llrs-roche!).

To make the timeline easier to remember, I would use the minor version as the timeline indicator.

Badge When to add the badge When to update/remove the badge
Experimental New function or argument After 2 minor version release or team decision, update to Stable
Stable After a new function or argument passed the experimental phase After 2 minor version release or team decision, remove the badge completely
Deprecated (Soft) Function has been replaced with something new, function or argument still work After 2 minor version release, update to Deprecated (Hard)
Deprecated (Hard) Once passed the Deprecated (Soft) period, function no longer working and not maintained After 2 minor version release, remove the function or argument from codebase

@llrs-roche
Copy link

@donyunardi teal has never had any major version release, only minor releases (major.minor.patch). This would make badges and changes take very long to update, imho. I didn't specify the type of release version used and perhaps the packages are on a state that major changes should go on with major version numbers. Did you mean minor version releases?

@donyunardi
Copy link
Collaborator Author

Did you mean minor version releases?

That's correct, I meant minor release. I updated the table.

@donyunardi
Copy link
Collaborator Author

donyunardi commented Jan 13, 2025

Team discussed the topic.
Badge guidance added to the CoreDev onboarding docs here.

Closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants