List or verify if a unit exist in a given implementation

[pierpaolocira]

Hi all,

I'm thinking to create a method to verify if a given unit is available for a specific implementation (or at least to list all available units for that).

In my mind it can be very useful in:

• applications where users have a greater flexibility (e.g. by defining new units, or directly typing a specific unit)
• the "Transform" phase of an ETL process
• applications that need to validate user inputs
• conversion application (both for listing available units, or for verify if the free-text wrote by the user comply)

I saw the AbstractPhysicalQuantity::unitNameOrAliasesAlreadyRegistered(), but it is protected, so inaccesible from the external classes.

I have several options:

1. to mark ::unitNameOrAliasesAlreadyRegistered() as public (in effect it is just a method for reading, so I think there ins't a too rigid cause to limit its visibility)
2. to declare a PhysicalQuantityInterface::unitAlreadyExists($unitInString); its implementation can "wrap" (adding just a couple of line of code) the unitNameOrAliasesAlreadyRegistered()
3. to declare a PhysicalQuantityInterface::unitAlreadyExists($unitStr); its implementation can directly access and loop over the the static::$unitDefinitions array
4. to declare a PhysicalQuantityInterface::listExistingUnits(); its implementation can returns an array of strings with all the existing units for that implementation

I know users can access to ::getUnitDefinitions() and loop over the UnitOfMeasure objects, but it requires more effort for a developer.

If you think this feature(s) can be useful I can quickly code it... just tell me what solution do you think is more suitable for the library (one between 1/2/3 or/and 4)...

[triplepoint]

Hello, and thanks for your effort.

Your option 1 seems like the easiest choice, but I don't know that it's as useful to an end user given that it takes a UnitOfMeasureInterface object. Surely most of the use cases would prefer a string parameter.

Option 2 runs into trouble in that the wrapper would end up having to instantiate a UnitOfMeasure object to pass into unitNameOrAliasesAlreadyRegistered(), thus tightly coupling these 2 classes together and losing the value of the UnitOfMeasureInterface interface (for what that's worth). This feels like the wrong way to go.

Option 3 seems like the best bet, though rather than directly accessing the $unitDefinitions property, we can just use getUnitDefinitions(). This tidies up the edge case of a quantity class that hasn't yet had it's default units initialized.

Maybe something like:

public static function unitExists($name)
{
    $units = static::getUnitDefinitions();
    foreach ($units as $unit) {
        if ($name === $unit->getName() || $unit->isAliasOf($name)) {
            return true;
        }
    }
    return false;
}

(I didn't test that, so if you do submit a PR please provide a unit test to be sure it works).
Also, feel free to implement it however you want; the above was just me thinking "out loud".

Having not looked at this code in a while, these static methods feel kinda messy to me. I remember the justification for implementing them (allow sharing newly-registered units between all objects of a given physical quantity), but it does fell cumbersome. Oh well.

Thanks again.

[pierpaolocira]

@triplepoint thank you for replying.
I'm working on this improvement, basing on your suggestion.

Just a couple of questions:

1. I think to declare it into the PhysicalQuantityInterface and implement it in the AbstractPhysicalQuantity... do you think is it correct into the project sources organisation?
2. What do you think about my point "4" (to also implement a method to return a list of existing unit)? I think it can be really useful in form-driven applications (or ETLs). Just imagine a case where a user can declare the unit of measure of him source file... a pre-validation form behaviour can generate a UI message prompting something like: "The unit of measure you declared is not valid. Please use one in the following set: aaa, bbb, ccc..."
3. The scenario about static functions you described is correct. Them should allow for sharing information between all objects, and should be also used when instanciating a new object is not really necessary (to save CPU/memory). Both of the case I described fall into the second case (e.g. Length::isUnitDefined($name) or Length::listAll() should be better than requiring to instanciate a Length object before proceeding... and those can also be called from non-static context). What do you think about?

Thank you

[pierpaolocira]

Hi @triplepoint... I had a bit of free time to finish... so if your reply to my previous questions are "yes", "yes", "yes" you can accept the pull request #62.
Obviusly, feel free to accept or deny it... in any case it also contains a temporary workaround for the issue #63.
Thank you

[pierpaolocira]

Hi @triplepoint... any news about this, #63 and the pull request?
Thank you

[triplepoint]

Doing a review now of #64, looks pretty good so far (minor formatting comment aside).

That glitch you reported in #63 is interesting. I'm going to spend a few minutes reproducing it on my local machine and see if there's an obvious fix. I don't really have a problem landing #64 as is, but it'd be nice to clear up #63 first if there's a quick fix.

[triplepoint]

Closing, having merged #64