WikiWatershed / model-my-watershed

The web application front end for Model My Watershed.
https://modelmywatershed.org
Apache License 2.0
57 stars 31 forks source link

Unit Conversion: Refactor #3034

Open rajadain opened 5 years ago

rajadain commented 5 years ago

Some ideas for cleaning up Unit Conversion refactor:

  1. Use Nunjucks Filters: Since we want to preserve the internal units and want to use user's preferred unit scheme only for display purposes, the unit conversion is pushed to the very edge of the render pipeline. In most current cases, it is implemented in the templateHelpers method. However, in many cases, this can be pushed further into the template itself, using the newly available toUnit filter, which could be used like this:

    <span>{{ value | toUnit('LENGTH_S') }}</span>
  2. Abstract Getting Unit Name: Often times, to get a unit's name, we must get the current unit scheme and then fetch it, like so:

    https://github.com/WikiWatershed/model-my-watershed/blob/834c40d4ca6664aefab6d45a7c87170434141a59/src/mmw/js/src/modeling/views.js#L833-L835

    This is repetitive and requires many things to import settings for no other reason. This should be replaced by something like:

    coreUnits.name('AREA_L_FROM_HA'),
    coreUnits.name('LENGTH_XL_FROM_KM'),

    which will return the correct name given the current scheme, looking it up internally, just as coreUnits.get does.

  3. Avoid Using Strings: Find a way to use enums for unit names, such as LENGTH_S, instead of using the strings, to prevent errors from mistyping.

  4. Add Tests: For example, to ensure that there is an equivalent for every unit class in every unit scheme. e.g. check that LENGTH_S exists in METRIC and USCUSTOMARY

  5. Return Direct Values: Currently coreUnits.get returns an object with the unit name and the converted value. The unit name is rarely ever used. Change this to only return the value, and remove all references to .value in the main codebase.

caseycesari commented 5 years ago

I agree that pushing the conversion to the template is ideal, since the conversion is just a user display preference. Ideally, the toUnit filter could be reworked to take the abbreviation of the unit, instead of the unit code. That's because the abbreviation of the unit is often available in the template context because it is used in the template for display, while the unit code has to be passed explicitly for use in toUnit. At least that's what I found from working on implementing unit conversion in subbasin.

rajadain commented 5 years ago

The reason I chose the unit code was because the unit abbreviation is different in each scheme, so the code serves as a uniform reference to the unit in all schemes. But I'm open to more efficient implementations, which I'm sure we'll discover as we use this more.