Remove unnecessary enums#220
Open
llucax wants to merge 10 commits into
Open
Conversation
Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>
Using the `ElectricalComponentOperationalMode` enum as an attribute is not very convenient. Users will need to check for multiple combinations when trying to find out if either telemetry or control is available, plus dealing with the possibility of an unspecified value, which should be rare, but possible. This new approach completely leaves the operational mode representation for the protobuf conversion layer, and maps different modes to new properties that are much safer to use. Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>
Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>
Electrical components are normally loaded from protobuf, and Python code should never create them, but get them from API services. We forbid constructing electrical components, but add a hidden escape hatch to be able to create them in `_from_proto()` functions. Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>
The idea is to get rid of all unspecified values in Python code, so this is a step towards that direction. Having a UNSPECIFIED value in that map is really not helpful, there is not much a user can do with that value. Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>
The problem was we were using a type too broad that required passing the category explicitly. If we use a type union of only the types we are going to test we can get also mypy validation for the ctor we are calling. Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>
Similar to operational status in electrical components, exposing an enum with the microgrid status is too low level and inconvenient to use. This class already exposed a `is_active` property, but an unspecified value produced a log warning, which is unsafer than it should, not allowing end users decide how to deal with it, and forcing a default that might not be suitable for all downstream users. This commit changes the `is_active` property to a function that raises if the value is unspecified. Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>
Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>
Like electrical components, this object is normally loaded from protobuf, and Python code should never create it, but get it from API services. We forbid microgrid objects, but add a hidden escape hatch to be able to create them in the `microgrid_from_proto()` function. Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>
Add also a release note about adding the new `electrical_components` package that was missing. Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>
llucax
requested review from
florian-wagner-frequenz
and removed request for
a team
github-actions
Bot
added
part:docs
llucax
requested review from
Marenz,
cwasicki,
eduardiazf and
tiyash-basu-frequenz
Contributor
Author
|
This might be a controversial change, but I think it is the right approach towards a more pythonic and save interface. The plan for the following PRs is, next do the same for stuff that exists in v0.4.0, that should get a deprecation path (I will probably backport these deprecations to v0.3.x and consider the v0.4.0 release a glitch and go for v0.5.0 for the next release). This includes removing Then the most disruptive and potentially controversial change: completely remove the Please comment if you have any concerns. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is the fist PR in what it will very likely be a series of 3 PRs to remove unnecessary enums and eventually all
UNSPECIFIEDenum values.Having
UNSPECIFIEDenum values is a common anti-pattern, and was only used so far because we started writing wrappers in a very 1-1 fashion. But Python abstractions should be more Pythonic and safer.The broad plan is to better abstract whole enums where possible, and to remove
UNSPECIFIEDvalues for the rest, preferring usingNone(or omitting the value completely) for the unspecified case.This PR mainly removes the
MicrogridStatusandElectricalComponentOperationalModeenums, using getters that can raise a newUnspecifiedValueErrorexception instead. Leaving the enums as low-level artifacts in the protobuf conversion functions only.It also introduces and related safety feature, forbidding by default the construction of
MicrogridandElectricalComponentobjects directly, as on the Python side they should only be retrieved from API services. This avoid users accidentally creating objects with unspecified values, another abstraction leak we want to avoid.