[idea] Rename module into teal_module (and also modules into teal_modules)

[pawelru]

A proposal: rename module() into teal_module() (and similarily modules() into teal_modules()).

Reasons:

• in line with argument data that accepts what teal_data() returns
• avoid confusion internally between modules argument value and modules() function

As a result we would be having something like this:

init(
  data = teal_data() |> ...
  modules = teal_modules(
    teal_module(...)
    ...
  )
)

I think it would more more consistent. Please tell me what you think about this.

This is a breaking change so it needs to be executed with appropriate deprecation strategy and so on.

[lcd2yyz]

While maintain neutrality on the proposal - should we prioritize this discussion and make a decision right away, so that if decision is a "go", we can squeeze it in before CRAN release?

[pawelru]

Yes - I think it's a good idea to prioritise it. I think it's rather a small item so I expect it to be a quick one.

[donyunardi]

I'm not too sure about this.

Changing the function name means that this will impact the modules as well.
For example:
https://github.com/insightsengineering/teal.modules.clinical/blob/0189b7acb644a05a1e34c9b1b37b6a1e775dadb0/R/tm_a_gee.R#L230

So we probably need to do a very thorough test of all modules. I just feel like it's adding more pressure to our releases.

Can we do this after release so we can have time to strategize this carefully?

[pawelru]

How about starting a deprecation cycle right now? So we will be having something like that:

teal_module <- (old module() colde)

module <- function(...) {
  deprecation_warning()
  teal_module(...)
}

This way we will be having old code fully functional (but with a deprecation warning).
Also please note that all the teal upstream packages will be looked into anyway so we can correct those calls anyway at that time.

[donyunardi]

I think this is possible, and I don't see any immediate risks.
Let's present this to the team to confirm in case I missed something.

[vedhav]

I like the idea. And, this would be the only time when we could do this breaking change without many users getting affected by it, as we release it for the first time on CRAN. And since it's going to be just soft-deprecation I don't see any risks.
When factoring in the doc changes and replacing in all the tm_*, TLG-C, and teal.gallery. I think we can pull it off in under a week. But, I feel we should do this fairly early. Doing breaking changes just before release is just a recipe for disaster.

[chlebowa]

I think "strategize carefully" and "squeeze it in" don't go together well.
Yes, now would be the perfect time to do it, should we choose to, but do we have to begin considering it at the eleventh hour?
Do we have time to properly consider the new name or will we just run with teal_module without a second thought?

Regarding consistency, we do have teal_data produced by teal_data and passed to data, true, but that is by accident - the old class was called TealData and we merely wanted to differentiate the new name. I am aware that we also have teal_slice returning teal_slice and teal_slices returning teal_slices but we should be careful to let coincidences like that dictate our further choices. Especially, and I cannot emphasize this enough, one such a short notice.

Introducing this change now can very well blow up in our faces and I will oppose it on this basis alone unless someone proves that it will only take one day to apply it throughout. This includes core packages, modules, gallery, catalog, unit test descriptions and all documentation.

[gogonzo]

Personally, I don't mind teal::module vs teal_module, teal::modules vs teal_modules. Since it is a teal we don't need to add extra teal_ prefix. I don't have a strong opinion though. The only function I would like to keep the same is teal::init, but it is more like a sentimental thing.

[vedhav]

teal_init would be ridiculous! And I would not change it cause we also call it rhino::init ;)
When you put it this way it does not feel very odd to call it modules and you're making me feel okay about sticking with modules. However, I still lean slightly towards the teal_modules naming because from a completely new user's perspective it would be easier to follow that teal_data and teal_modules are the two most important things init needs.
Is it worth this effort?
I'm not sure. The only thing I know for sure is that I'm only okay doing this now and would hate if we do it later in the future when there is tangible adoption of the framework. That is why I am leaning slightly towards this so we don't have regrets.

[m7pr]

Yeah, so we either do it now or never. There is slight benefit of having teal_modules instead of teal::modules which is in terms of consistent naming convention with teal_data and teal_slice (even if it's just a coincidence) and it maybe removes the confusion from which package it originates, but that's all.

From the first glance you might think this is just a global substitution in teal package and a depracation note, but then you need to go over the whole framework as @chlebowa mentioned

packages, modules, gallery, catalog, unit test descriptions and all documentation.

I'm fine with the change, but I wouldn't be comfortable doing it on my own and risking on my own the verification of all other places, just to change a name, which probably in most cases is already pre-fixed with teal package name. So if someone is willing to take the responsibility - good luck - but I would rather not do that if I were the only developer on a project, moments before the final release of the whole framework.

[donyunardi]

Thanks for the input, everyone.

I agree that the teal prefix would make the code look consistent, but I also can't recall an instance when a user approached me about confusion with the current naming convention.

However, I do share the sentiment about how this change could affect our timeline and workload if something unexpected arises. Ultimately, I think the timing is not ideal, and I don't want to gamble on this.

We still haven't decided the roadmap to teal 1.0, and there's a chance that we will be doing another big update. Perhaps we can introduce this change then. We can have a longer deprecation period, allowing us to have ample time to discuss and double-check everything.

[lcd2yyz]

Thanks everyone for your input!

Is it worth this effort?
I'm not sure. The only thing I know for sure is that I'm only okay doing this now and would hate if we do it later in the future when there is tangible adoption of the framework.

This is exactly my thoughts also! Which is why I hope we can make a "go" or "no-go" decision now, and with the understanding that we will have to live with whatever the consequence of this decision for a while (until the next big refactor on teal itself, whenever that may be).

Summarizing everyone's inputs:

• From technical-need perspective: no strong need or strong preference for one way or the other.
• From technical-implementation perspective: strong concerns regarding meeting a hard-fixed timeline and release schedule, low confidence on success.
• From business-need perspective (PO assessment)
  • No user feedback regarding modules function naming thus far, as @donyunardi also mentioned (neutral)
  • Perception of better consistency on function naming, if changing from module into teal_module (plus)
  • Another deprecation to learn and adopt for all existing teal users (minus)
  • The "plus" and "minus" roughly cancels out => so overall, no strong business need

Taking in all of the above, the potential benefit of renaming does not seem strong enough to outweigh the difficulty of successful and timely execution, so I would conclude it as "no-go", and we will be sticking with teal::module and teal::modules for the foreseeable future.

[averissimo]

Discussion resurfaced on this comment: #1357 (comment)

Regarding teal_transform_module

[chlebowa]

Well, it's not two weeks before release this time 😂