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

Formulas: add the SWITCH function #5407

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

laa-odoo
Copy link
Collaborator

@laa-odoo laa-odoo commented Jan 6, 2025

Task: 3829128

@robodoo
Copy link
Collaborator

robodoo commented Jan 6, 2025

Pull request status dashboard

@laa-odoo laa-odoo force-pushed the master-formula-add-switch-function-laa branch from e0c1b12 to 5314a0c Compare January 6, 2025 09:23
src/functions/arguments.ts Outdated Show resolved Hide resolved
src/functions/arguments.ts Show resolved Hide resolved
@laa-odoo laa-odoo force-pushed the master-formula-add-switch-function-laa branch 6 times, most recently from 8d5b68a to 80d0c65 Compare January 10, 2025 12:31
@laa-odoo laa-odoo force-pushed the master-formula-add-switch-function-laa branch 3 times, most recently from 13c00a8 to 0b34668 Compare January 14, 2025 12:22
laa-odoo and others added 2 commits January 14, 2025 17:10
We want to introduce new formulas whose definitions may deviate from traditional patterns.
For instance, we might develop a SWITCH formula where an optional argument appears after
repeatable arguments. We also foresee future formulas with mandatory arguments following
optional arguments.

However, our current logic cannot properly map values to their corresponding arguments.
Moreover, we are not entirely sure what other “exotic” configurations might exist and how
they would behave.

This commit introduces a completely new way of mapping values to arguments, making it possible
to handle any configuration—even the most unconventional. Henceforth, we do not have to worry
about the order in which - mandatory, optional, and repeatable - arguments appear; they can be
mixed in any manner the developer chooses.

The only constraint we enforce is that repeatable arguments must be declared in groups, and, if
repeatable arguments are present, their total number must exceed that of the optional arguments.

These changes also impact the formula composer: By supporting formulas with mandatory or optional
arguments placed after repeatable ones, any given argument value the user enters might correspond
to either a repeatable argument or a mandatory/optional argument. The argument to focus on in the
composer thus depends on how many arguments are ultimately passed to the function,
requiring us to potentially focus multiple arguments at once.

Lastly, we addressed a conflict with an existing “exotic” formula, SORTN (from Google Sheets),
which originally included two optional arguments alongside two repeatable arguments.
To make these new rules work, one of the optional arguments was converted into a mandatory argument.

Task: 3829128

Co-authored-by: anhe-odoo <[email protected]>
@laa-odoo laa-odoo force-pushed the master-formula-add-switch-function-laa branch from 0b34668 to 92b61aa Compare January 14, 2025 16:11
@laa-odoo laa-odoo changed the title WIP Formulas: add the SWITCH function Jan 16, 2025

this.functionDescriptionState.functionName = parentFunction;
this.functionDescriptionState.functionDescription = description;
this.functionDescriptionState.argToFocus = description.getArgToFocus(argPosition + 1) - 1;

const isParenthesisClosed = !!this.props.composerStore.currentTokens.find(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can use some :)

@@ -221,12 +221,20 @@ export const SORTN: AddFunctionDescription = {
compute: function (
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should remove the isExported (or set to False)

Comment on lines +139 to +142
* The tables are built based on the following conventions:
* - `m`: Mandatory argument
* - `o`: Optional argument
* - `r`: Repeating argument
Copy link
Contributor

@hokolomopo hokolomopo Jan 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't read the details of the code, but are we not taking a wrong path here ? I'm not really sure why we are trying to handle exotic arguments combinatations (do they even exist in practice?) at the price of an argument combination that we know exist and was working fine (SORTN).

Reading only the commit msg and this function docstring my main takeway are:

  1. SORTN arguments (FUNC(m, o, o ,o, r, r)) sounds entierly reasonable. I don't think we want a solution that does not support that.
  2. My very thrustworthy source (chatgpt) told me that mandatory arguments are always placed before optional arguments. Idk if he's right or not. And even if they are functions that do that, isn't the correct syntax to have empty arguments in the arguments list ? eg. your configuration 4 we would not write =FUNC(m1, m2, m3) but =FUNC(m1, , m2, , m3) that way there is no confusion.

We should talk about this IRL, there's most likely tons of subtleties that I'm missing

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be honest, I'm not entirely sure about the proposed solution here either.

It also bothers me for the point you raised, but I’m taking this opportunity to put in writing what has been discussed regarding this task.

To try and answer your question, the goal is not to specifically handle exotic cases, but the proposed solution allows us to manage all corner cases without having to constantly question how far we should go (or stop) in processing the arguments—at least, IMO, I don’t really know.

How did this reflection arise? It started from the need to implement the SWITCH function (like a switch case). It turns out that this function allows adding an argument at the end in addition to the repeatable arguments. This final argument corresponds to the default return value. Thus, the function follows an exotic pattern: [m, m, m, r, r, o]—exotic because an optional argument appears after repeatable arguments.

I call it exotic, but in reality, Google and Excel implemented SWITCH with the following pattern: [m, m, m, r, r]. It’s only through the user interface that the first repeatable argument is displayed as both repeatable and optional. From a user perspective, I find this quite confusing, as the UI keeps prompting the user to enter another repeatable parameter even after the optional argument.

Additionally, following the same approach as Excel and Google would have required finding another way to verify that the number of arguments passed during compilation is correct. With Google and Excel’s approach, it is no longer sufficient to simply apply a modulo operation on the number of repeatable/optional arguments to check whether the provided argument count is valid.

So, we asked ourselves if there was a solution that could handle cases like [m, m, m, r, r, o] to avoid compilation issues while also ensuring a proper representation of argument insertion in the UI.

This reflection also took into account that other formulas, such as LET and LAMBDA (which we don't yet support), have their own unique patterns: [m, m, r, r, m] or [r, m].

Considering all these cases - [m, m, m, r, r, o], [m, m, r, r, m], [r, m], and [m, o, o, o, r, r] (for SORTN) - there is probably a way to handle them properly. However, we don’t know if other exotic patterns should also be considered. For example, given the nature of the cases mentioned, could a pattern like [r, r, r, m, o] exist? It doesn’t seem far-fetched considering the other cases.

So, should we write specific code that we modify/restrict for each particular case we encounter? I don’t know.

In any case, the proposed solution aims to be universal and allows us to avoid worrying about a specific order of arguments. However, there are still some rules to follow, which conflict with SORTN.

Regarding SORTN, it turns out that this function was introduced by Google, not Excel. Since Google isn’t as "serious" as Excel, I concluded that they didn’t really know what they were doing and mistakenly defined their arguments, failing to make the second argument mandatory. This actually works in my favor, as it aligns with the rules of the new algorithm.

Now, I’m well aware that this solution could create confusion, such as in configuration 4, which you mentioned. However, configuration 4 has no functional relevance here—it is only meant to illustrate what the algorithm implies.

It’s worth noting that implementing such configurations will remain the responsibility of the formula developer rather than the end user. There is still a layer of abstraction before users encounter such cases and get confused about how argument interpretation actually works.

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

Successfully merging this pull request may close these issues.

4 participants