Supersede derive_var_merged_exist_flag()

[manciniedoardo]

Originally posted by @kaz462 in #2191 (comment), suggesting to deprecate rather than superseding

Supersede derive_var_merged_exist_flag

the new function derive_var_merged_ef_msrc covers the functionality of derive_var_merged_exist_flag() - so we can supersede derive_var_merged_exist_flag() .

Definition of done

The function derive_var_merged_exist_flag() should be superseded (note: this is different from deprecation because we just direct users to use a newer function but do not remove the older one) so that it appears here). See an example of how this has been done for derive_var_dthcaus() in the @description, @family and @keywords field.

[manciniedoardo]

A cursory review of the usage of derive_var_merged_exist_flag() showed that we don't use it that often in our code/repo. So I am happy for this to be superseded in favour of derive_var_merged_exist_flag(). I see no reason to deprecate at the moment due to our 1.1 strategy.

We would also want to replace mentions of the former with the latter anywhere they appear, and try make sure this also happens across extension packages.

[bundfussr]

Is this a general decision? I.e., should we supersede all functions with a single source if a corresponding function for multiple sources is available? For example, derive_vars_extreme_event() covers derive_vars_merged().

[manciniedoardo]

@bundfussr no not a general decision derive_vars_merged() is a nice simple function so i wouldn't supersede it. It can be ok to have overlaps in functionality within admiral.

[bms63]

I'm actually a little confused on what is the purpose of the issue. It reads as a question currently. Is this Issue to discuss this or are we going to really supersede the function. ?

Could the original issue to be updated with what we are trying to accomplish here please? Definition of Done. Since we have good first issue tagged - we should at least link the dev documents on our supersede/deprecation strategy for new folks and provide any other helpful tips for this task.

[manciniedoardo]

@bms63 good points, i was a bit hasty while making the issue. I made some updates.

[rossfarrugia]

Is there anything to be said about optimisation here? e.g. I imagine derive_var_merged_exist_flag() runs quicker than derive_var_merged_ef_msrc() as its simpler. I remember that was an argument for keeping derive_vars_merged() when derive_vars_joined() first came, as technically the joined functions could have been used here all the time. Just one thing to consider from past old discussions around deprecation strategy I remember - feel free to ignore though as this one doesn't seem major impact.

[Fanny-Gautier]

I proceeded with the updates in terms of documentation.
Does it also need to be updated in the functions wherever it is used? I mean to replace derive_var_merged_exist_flag() calls by derive_var_merged_ef_msrc() calls, e.g. as in below screenshot.

[Fanny-Gautier]

I have been through the deprecation discussion. Could you please confirm whether I should also add some warnings regarding the future deprecation of the derive_var_merged_exist_flag() function ? Thanks.

[bms63]

• The vignettes using derive_var_merged_exist_flag() need to be updated to use the new function
• The stuff with @inheritparams that is a bit tricky - can you look to make sure that the new function has all the same arguments. We either will update to use the new function, replace with relevant arguments, or pull from similar function
• The other two functions in screen shot looks like they use the derive_var_merged_exist_flag() under the hood. Can you see if the new function can replace that without impacting anything?

As far as deprecation. This function should emit a message for the next two years. In the third year we will start to emit a warning and at the end of the third year we will fully remove the function. I'm actually unsure how we should approach that at this moment. I will get back to you.

The above three tasks are a bit involved. Let me know if anything needs to be clarified.

[Fanny-Gautier]

How derive_var_merged_ef_msrc() can replace derive_var_merged_exist_flag(), while derive_var_merged_exist_flag() is used within derive_var_merged_ef_msrc() ?

Does derive_var_merged_exist_flag() still need to be deprecated in the future ?

[bms63]

Well there we go - I'm not sure there is grounds then to supersede or deprecate the function :)

[bms63]

@manciniedoardo what was the rationale for the supersede/deprecation? I don't think we should go ahead with this one

[manciniedoardo]

@bms63 the original rationale came from this comment by @kaz462. Thanks for investigating this further @Fanny-Gautier - I agree that, on review, it would be best not to go ahead with this supercession given is used within derive_var_merged_ef_msrc() itself depends on derive_var_merged_exist_flag().

[bms63]

Thanks for investigating @Fanny-Gautier!! I'm going to close this issue out, but it does show that we need tighten up our deprecation strategy (TBD).

Let us know if any other programming docs that you found confusing and could use some refresh.