Change attribute getters and setters into methods

[sveinse]

This PR adds access methods, get_* and set_* where attribute getter and setters are used. The old getters and setters are kept intact and working, but using it prints a warning in the log if used.

The motivation for the proposals are:

• IO blocking operations are opaque with getters/setters. That var.raw generate blocking IO is unexpected and hard to track (and somewhat unconventional)
• Getter and setters cannot be used when using async. Having dedicated access methods allows similar usage of API between the regular blocking API and the async implementation

Having getters and setters have some upsides too:

• Getters and setters are very compact: var = param.raw and param.raw = 42 is neat.
• This is the established API in canopen

The intention by adding this PR is to try it on for size. @acolomb expressed skepticism for it in #355, while @christiansandberg expressed interest in #271. Perhaps there is a middle ground to be gained?

[acolomb]

So much noise just to use uglier names than before... doesn't make me happy :-(

Are you sure there is no sensible way to re-use the property accessor functions in an async context without declaring everything twice? What's the problem with those actually?

[acolomb]

Well, in my eyes this really kills the spirit of the Python library, it's just more reading without any more information. The whole point of properties and their accessors is that the (average) API user should not care about where it comes from, they just want the value. In this case, it is stored somewhere else, so must be fetched.

Of course for advanced use cases, blocking I/O functions should be clearly noted. I'm fine with having variants that will work with async etc. We just shouldn't burden the user with more complicated names, or warn them when using a perfectly fine (synchronous, blocking) API.

[acolomb]

If we want to go this route, this adds another layer of indirection, which has a small runtime penalty. Couldn't the properties be created by specifying the get_*() and set_*() methods directly as fget and fset (if that's what they're called...) parameters? If the decorated method is just a wrapper itself, there is no real advantage in using a decorator anymore.

[acolomb]

Please not this :-) It'll cause a truckload of log spam in existing code, without anyone digging in to fix things as long as they keep working somehow.

[sveinse]

The DRY principle in python guides us not to, well, repeat ourselves. Having an API with multiple set/get methods is in my opinion not ideal and thus I wanted to propose to inform the user that the property based attr access is no longer the preferred way to do it. However, I do acknowledge that this is how this library has been designed. I won't argue if we decide that either get methods or property based access is supported without any warnings.

[acolomb]

Breaking existing code is even worse than repeating yourself. But I'd still like to see a solution where we don't need any repetition, or at least reduce it to the absolute minimum.

Haven't thought it through, but I imagine some very basic code like this to "extract" the existing functions:

class NmtBase:
    @state.setter
    def state(self, new_state: str):
        # ...    
    set_state = state.fset

My experience with asyncio is nearly nil though, so no clue what it would take to make this async-compatible then.

[sveinse]

@acolomb "Are you sure there is no sensible way to re-use the property accessor functions in an async context without declaring everything twice? What's the problem with those actually?"

This construct doesn't exist for python, so properties are no-go for async.

# Not valid python
@property
async def raw(self):
    return await self.aget_data()

For async it will have to be a dedicated getter function

async def aget_raw(self):
    return await self.aget_raw()

The fundamental design implication with async is that one can only call async and regular from async, not the other way around. I.e. regular functions, like property methods, cannot call any async functions. The following construct will not work:

async def aget_data(self):
    # This does something back-end which is async
    return 42
@property
def raw(self):
    return aget_data()

# Will not return 42
v = myobj.raw

[acolomb]

We're pretty clear on the fact that the properties won't work with async. My point is that the underlying "raw" methods do exist though, so we should wrap those into async variants aget_...() instead of the other way round. And to do that, best use the smallest possible amount of code, basically just putting a wrapper around with a different name. That could even be done with a decorator, similar to what property() itself does. Remember, in the end all you need to do is write an expression that evaluates to a functor (IIRC).

[sveinse]

There isn't a need to change the property based access methods in canopen for async. The current blocking design can perfectly live next door to a method-based async variant. But as stated in the opening comment, my opinion is that I don't like blocking IO being implicitly called when accessing an instance attribute. That is unexpected and afaik unconventional. Coupled with not being able to use it with async, my proposal with this PR is to try to close the gap between the two APIs to make them more like and consistent. But they don't have to be.

Perhaps we could consider continuing the async-part of this discussion in PR #359, where I'm proposing to add asyncio support to canopen? (PS! Once this PR is resolved, I will apply the same to the async port, so it reflects the decision for the non-async parts of canopen.)

[acolomb]

Sure, we can look at this in isolation from async.

But as stated in the opening comment, my opinion is that I don't like blocking IO being implicitly called when accessing an instance attribute.

You definitely have a point there, and when researching the topic of "async property setters", one quickly finds references to PEP-8 that basically states properties should be cheap to access, without unexpected side effects / delays. HOWEVER, breaking the library API users' code by calling something deprecated requires a very good migration plan IMO. So far, the only plan is "spam them with log warnings, someone will take care", which essentially breaks the code for good because no-one will invest such a high effort to adapt and TEST a code-base just because of some unnecessary renamings that make the code even more unreadable (for the average python-canopen user I have in mind, who is oblivious even to the fact that code takes time to run).

By the way: The basic assumption for many industrial field bus systems seems to be "values are simply there -- read them, modify them, something will take care to sync the changes to some other device". As long as this happens in a dead-simple input-process-output loop, the fact that some signals travel longer is in fact transparent, but the possible performance is also terrible, not even thinking about async programming or parallelism. So the existing library API caters well to an audience with this mind-set, although access is actually immediate and blocking. The described pattern is actually exactly what PDOs and SYNC objects are there for. IMO, that's also the part where proper asyncio integration would be much more valuable than for SDO or NMT access.

[sveinse]

In summary I think the biggest argument for leaving the current behavior as-is is that this library has already set an precedence, although perhaps not perfect. On that account, I see arguments to cancel this PR.

OT for this PR, but I'm trying to consider it for the bigger picture: The canopen-asyncio port must have aget_*() methods for async as previously discussed. It is convenient to have the corresponding get_*() methods available, as the coding between async and non-async uses become similar, e.g. when reading PDO Variables which is not async. Perhaps this is a factor to keep canopen and canopen-async as two separate libaries (#359)?

[acolomb]

I'd very much like the two versions getting merged into one library someday. Parallel development creates friction and costs time which can be spent on more worthwhile changes.

What about my proposal in #359 (comment)? I think as a user, you either work with the existing API or the async methods. So there is no point in trying to make them look exactly the same except for an a. Can't we just use async variants of the .read() / .write() methods and be happily async-compatible? Is there any need for other setters / getters beside those two functions? Of course that's not all there is to asyncio in this library, but probably the most frequently used bit. And I find the method naming particularly concise, await sdo.aread() (or read_async()) seems pretty clear as a deferred action.

So yes, I think this PR's approach should be cancelled. Leave the existing API and implementation as it is. Let's try to minimize the changes needed for async support.