IDAES / idaes-pse

The IDAES Process Systems Engineering Framework
https://idaes-pse.readthedocs.io/
Other
216 stars 234 forks source link

Reduce the number of magic string constants in code #669

Closed andrewlee94 closed 1 year ago

andrewlee94 commented 2 years ago

From a comment in #620:

@dangunter:

There are lots of magic string constants like "mw". imho, most of these should be defined as constants. I tend to create a class (e.g., > Constants) and define class variables with same/similar names, so:

param_dict = {"mw": base_units["mass"]/base_units["amount"]} would become

class Constants: mw = "mw" #: this is also an opportunity mass = "mass" #: to comment on what these
amount = "amount" #: actually 'mean'.

param_dict = {Constants.mw: base_units[Constants.mass]/base_units[Constants.amount]}

If the wordiness bothers you, you could alias the class to a short abbreviation like _C

In the snippet of code above, there are actually two sets of magic string constants and I think that if we were to implement thus we would want to create separate classes for each:

  1. A set of standard names for properties (probably part of property_base.py)
  2. A set of recognized units of measurement (probably part of property_meta.py)

Do others have thoughts on this, and how important do we think this is?

andrewlee94 commented 2 years ago

One thought that did just cross my mind is that this could make it easier for us to document these things, as we could use Sphinx to autogenerate the docs based on the class of string constants,

eslickj commented 2 years ago

I like the suggestion. Just make some classes that are like Enum but with string constants. Maybe we could have it so there is an easy method to check if a string const is defined in the class for argument validation in case people supply strings directly.

jsiirola commented 2 years ago

One of the things we have used in the past was a string-variant of Enum (see, e.g. https://github.com/Pyomo/pyomo/blob/2ad064be83da890f07fb1cd6109aca6ed4916c69/pyomo/core/expr/calculus/derivatives.py#L16-L27):

class BaseUnits(str, Enum):
    mw = 'mw'
    mass = 'mass'
    amount = 'amount'

    def __str__(self):
        return self.value

This makes an enum that provides a lot of backwards compatibility with strings. In particular, the multiple inheritance and __str__ overload causes the following to work:

>>> 'mass' == BaseUnits.mass
True
>>> BaseUnits('mass')
<BaseUnits.mass: 'mass'>

Plus, printing out the unit produces the expected (old) string:


>>> print(BaseUnits.mass)
mass
dangunter commented 2 years ago

I think the str, Enum inheritance is a good idea. I don't know exactly what @andrewlee94 is thinking with the docs, but it should make it easier to document for sure.

andrewlee94 commented 1 year ago

With #995 finally merged, I am going to call this done for now.