Introduce inheritance to the Parameter structure

[gabrieldutra]

What type of PR is this? (check all applicable)

• Refactor

Description

This is a start of a draft of what I was thinking of how parameters should be in code as we are increasing complexity for them. So far basically started with inheritance, the "abstract" Parameter holds the default methods that will be replaced as needed for the parameters.

This way it's easier to control Parameter valid values and introduce new features to existing parameters or add new types.

Opening this already to gather comments from your ideas.

Fixes #3769

[gabrieldutra]

So far moved all code to their separate files, I still want to revise the methods names (there are several "value" attributes and it would be nice if they were a bit more descriptive on what they do, or at least have comments). Also need to see if something else can be cleaned up. Will continue tomorrow. @kravets-levko and @ranbena lmk if you have inputs here :)

[gabrieldutra]

As Cypress pointed out, this doesn't work with inheritance 🙂

[gabrieldutra]

Review of existent values in the Parameter Structure:

• param.value: Value stored in DB. It's also used as Parameter Value for Scheduled queries that contain parameters. Can be String (dates are formatted), integer or a List in case of Multi-select parameters.
• param.$$value: Value used internally for Inputs. Guess it should not be stored in DB (not sure if it is now). Can be anything, dates are moment instances in this case.
• param.getValue(): Execution Value. It's the function that's is called to get Parameter value before execution. Usually the same as param.value, with some differences for example for Dynamic Values (they are solved into actual dates here). Thinking about renaming it to getExecutionValue or sth else that will make more sense.
• param.normalizedValue: Getter for $$value.
• param.pendingValue: Pending value for apply changes. Same type of value as $$value (except for Dynamic Values, but I'll review that too)
• param.dynamicValue: Returns the Dynamic Date or Date Range, which is basically an object with a Label and a function that generates moment values. I'm thinking about merging it into $$value.
• param.setValue(value): Takes anything as a value, normalizes and modify both the value stored and the internal value.

From those need to make sure only param.value is stored

[gabrieldutra]

Dynamic Values merged in $$value (also added DynamicDate[Range]Type PropType).

Regarding values, this is what I've been thinking: (once defined I would also put as comment in the "abstract" Parameter class)

• this.value (attr): DB value
• this.$$value / this.normalizedValue (attr): Input value, can always be obtained through the this.normalizeValue(value) method;
• this.normalizeValue(value) (func): Normalizes input values. Should turn anything that makes sense into a predefined type (e.g.: turn an integer into a String for TextParameters) or to null in case it can't. In the basic form, it needs to have instructions to normalize both this.value and this.$$value types.
• this.setValue(newValue) (func): Defines both this.value and this.$$value as the normalizeValue form, should be overridden in the cases where this.value is different.
• this.getExecutionValue() (func): Execution value. Basic form is just returns this.value. Overridden when it should have any other special treat (e.g.: dynamic values or multi-select).

[ranbena]

I like it a lot. So glad you're doing this :)

Thinking about this, in order to make things unmistakable, wdyt of having one value only which can be converted into different formats via static method param.formatValue?:

• param.value - for storage
• param.formatValue(param.value, FORMAT.DISPLAY) - for humans
• param.formatValue(param.value, FORMAT.SQL_QUERY) - for sql server

It's long, might not be performant, but I'd like to find ways to minimize confusion above all.

[gabrieldutra]

wdyt of having one value only

Definitively more readable, the FORMAT.TYPE isn't usual unless you consider formatValue as a "private" method. My other concern was on the various possible executions of the function to get the Input Value (that's why I left $$value in there). However I liked it, I'll maturate this idea and follow my conclusions :)

[ranbena]

the FORMAT.TYPE isn't usual unless you consider formatValue as a "private" method.

Yeah I guess it's meaningless.

My other concern was on the various possible executions of the function to get the Input Value (that's why I left $$value in there).

You mean performance, yeah? Maybe memoize?

[gabrieldutra]

@ranbena I'm actually thinking about keeping them as variables (updated on setValue) and just rename them to something that will make more sense (or add helpful comments). I think the memoize thing could work, but also seems to me we would need to reset at every setValue (as it can mess up for the values that depend on more than the function parameters).

@kravets-levko lmk if you have other suggestions for the parameter structure.

[ranbena]

@gabrieldutra I'm working on #2721 and I wanna base it on this PR.
Though I'm missing a param feature - isEmpty for the pendingValue.
If that's straight-forward, awesome.

If not, maybe consider making pendingValue be a Parameter instance (instead of normalizedValue), which will allow its isEmpty out of the box.

[gabrieldutra]

Cool, @ranbena, I actually want to move forward and merge this soon anyway (will sync it today).

For the isEmpty thing I believe we can have it as a static method (constructor.isEmpty(value)) and so we would have it available at any given moment?

[ranbena]

I’m with you on that 👍

Regarding isEmpty - don’t you think it should be allowed to be set by each param type?
Or is it always gonna be ‘not null’ ‘not empty string’?

[gabrieldutra]

@ranbena I've realized I could generalize the "empty" concept like this (ideally invalid values are turned into "null")

[ranbena]

It looks great 🎉
Two non-must comments.

[ranbena]

toNumber converts empty strings to 0, but I think it should be normalized to null instead.
How about:

import { toNumber, trim } from 'lodash';

normalizeValue(value) {
    if (!trim(value)) {
      return null;
    }
    ...
}

[ranbena]

Suggesting addition:

isEmptyValue(value) {
  return !trim(value);
}

[arikfr]

🚢