Use enums for constants

[Booplicate]

Closes #1741, went a bit further and converted all static classes to enums. This should provide type safety while maintaining compatibility with raw ints and strings.

Changelog:

• ModbusStatus > inherits from int and enum.Enum
• Endian > inherits from str and enum.Enum
• ModbusPlusOperation > inherits from int and enum.Enum
• DeviceInformation > inherits from int and enum.Enum
• MoreData > inherits from int and enum.Enum

[janiversen]

You need to resolve the linter errors. Changing to upper-case seems most correct, but there were no lint problems before, so maybe it is due to how you changed the classes.

[janiversen]

This should be replaced by the system constant.

[Booplicate]

Not sure I understood, do you mean sys.byteorder?

[janiversen]

Upd.

I updated the tests, at first I wanted to replace

assert str(handle) == "ReadDeviceInformationResponse(1)"

with

assert handle.read_code == DeviceInformation.Basic

but there's already a test for that, and I don't see a reason to test value of read_code on encode. I just removed them.

Lint fails and I agree constants should be in UPPER_CASE, but for compatibility we have to leave them as is.

The change you wanted to make, is quite a different test, the test is checking the string representation of handle, not an individual component.

There are no compatibility issue for a couple of reasons:

• these constants is for internal use, not defined in the API
• v3.5 is comming soon, and in that API changes (documented) are allowed
• We never merge a PR without CI being green.

[janiversen]

@janiversen just to note, at least Endian looks like part of the public API. One way we could handle this is by suppressing the warnings and adding aliases for compatibility.

However if you're fine with breaking things, then I will just rename as you proposed.

It is the default in many payload calls, and therefore it will not change anything if using the "official" constant instead.

If you rename things that are real part of the API (meaning people will have to change their app) then add a line in API_CHANGES.rst.

Yes I am happy with renaming, we have had changes like this in every 3.x version but never in 3.x.y versions.

[janiversen]

restarted job.

You can easily restart yourself:

    git commit --amend
    git push -f

[janiversen]

The only bigger item is the remove of init.

Please add a short description in API_CHANGES.rst (section 3.5) that constant now uses enums and are capital letters.

[janiversen]

Why should we permit objects ? I do not see the need in contrary.

[Booplicate]

Enum members are singletons by default, we don't need any special logic to guard them, Python handles that for us.

[janiversen]

It does not prohibit that I can assign the class to a variable and make changes, with unexpected results.

And as far as I know, if you instanciate the class you get a new set of class variables (elements of enum).

[Booplicate]

And as far as I know, if you instanciate the class you get a new set of class variables (elements of enum).

Actually, I believe no, class variables are static. You can change them, but restricting __init__ won't prevent that anyway.

I think I should clarify, __init__ is used to initialise enum members, not the enum itself.

Enums are using metaclasses underneath. Say, if somebody does ModbusStatus(0xFFFF), they will get ModbusStatus.WAITING, because enum members are singletons.

[janiversen]

We should not permit to instantiate any constant class.

[janiversen]

A question, why do you inherit from int, str ? I do not see anything in the class that makes use of it...you do not make a special int.

[Booplicate]

It's the way to create derived enums. By default enum members are just unique constants with some value. However, in our case the values have meaning (like modbus code), so we want to control what the values are, and we want to use the enum members as the values. Thus, for hex constants I used int, for string, I used str.
For example, in case of ModbusStatus, we want

assert ModbusStatus.WAITING == 0xFFFF

Without the inheritance, we'd have to do

assert ModbusStatus.WAITING.value == 0xFFFF

which is more error-prone and less convenient to use.

[janiversen]

Good point !

Apart from when you look at comm_types in transport, that class inherits "class comm_types(Enum)" and the values are addressed directly as comm_types.TCP.

Edit:
---> My mistake forget it.

[janiversen]

A short example why you need to disable init:

Python 3.11.4 (main, Jul 25 2023, 17:36:13) [Clang 14.0.3 (clang-1403.0.22.14.1)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> from constants import ModbusStatus
>>> test = ModbusStatus(ModbusStatus.WAITING)
>>> test.WAITING = 0
>>> print(test.WAITING)
0
>>> print(ModbusStatus.WAITING)
65535
>>> 

Having different values for the same constant are likely to cause problems (and have done so).

[Booplicate]

I will see if it's possible to change behaviour of __init__, but I doubt so.

I believe that LS/typechecker/IDE should prevent that kind of errors:

because you can override even if you don't init:

test = ModbusStatus.WAITING
test.WAITING = 0
assert test.WAITING != ModbusStatus.WAITING

[janiversen]

IDE is no good to us, we depend on CI to be the final judge.

I simply do not like to remove this kind of protection unless it is absolutely nessecary , it will not provide 100% protection, just like constants can be assigned to, but it's better than nothing. I made the example to show that your argument about singleton doesn't not hold in this case.

[janiversen]

We are getting close to release time, so if you want this in please solve the outstanding issue, and request a new review.

[Booplicate]

I added the changelog.

@janiversen, sadly, I think it's impossible to disable __init__ because enums use it to initialise its members. I tried to force it to only being callable once, but that doesn't work well on different py versions (and generally looks messy).

If you have any other ideas how to implement a runtime check like that, I could try to do it, otherwise it's a question if you're willing to sacrifice that over type safety.

[janiversen]

Did you try to call super().init()

[janiversen]

the init() is only called if you instanciate the class, so something sounds wrong.

[Booplicate]

Enums use metaclasses, so logic from ordinary classes cannot be applied, __init__ is being called for each enum member. I tried playing around with removing/changing __init__ and __call__ (they use both!), but nothing seems to result in the behaviour you want (most result in exceptions of sorts).

If you try:

class Example(enum.Enum):
    def __init__(self, value):
        raise Exception("need __init__")
    A = "A"
    B = "B"

You will get Exception: need __init__ right away.

[janiversen]

Seems you are right.

[janiversen]

LGTM, thanks

Looking forward to your next PR.