Deprecate attribute based IO operations, like v = var.raw

[sveinse]

I'd like to propose to deprecate the uses of attribute based operations, e.g. value = var.raw or var.raw = 32 throughout canopen. The problem with the existing implementation is that some of these attr getters and setters end up in IO operations towards the CAN system. This makes it very opaque if the attr ops are "local", i.e. fetching data from the class, vs. doing remote operations. A more explicit API will make it easier to understand when IO happens and it makes the code more portable.

I have created a PR-ready proposal here:

https://github.com/sveinse/canopen-asyncio/tree/feature-deprecate-attr-ops

Compared to canopen master:

master...sveinse:canopen-asyncio:feature-deprecate-attr-ops

All attribute getters and setters in canopen are replaced with explicit methods, .e.g. var.get_raw() and var.set_raw(). The proposal doesn't alter or break the existing attr-based API, but it prints a deprecation warning if used.

I am well aware and understand that this proposal might be controversial. There are advantages to keeping the attr based operations. Its simple and it allows good duck-typing for e.g. local and remote nodes and it has been this way in canopen for a long time. However, this methodology is very implicit. It is difficult to spot when an attr operation results in blocking IO. This type of change will become necessary when porting to async, since async getters and setters is not a thing.

Should I proceed with creating a PR for this change, or is there need to discuss the topic?

[acolomb]

I'm strongly against creating methods named get_something() and set_something() when we do have proper properties in Python. The only understandable exception to that rule I see is when wrapping a library API to keep the names compatible, although a good wrapper will even then provide properties to shorten the code calls.

I do understand your concern, but would like to keep the properties for the most simple, readable way to express what is being done, still in a synchronous fashion. The fact that it involves blocking for a response is something that I suppose most users actually welcome, such as used in a script. The performance oriented use cases where thought has been put into how concurrent operations cooperate is the advanced, less common way where I would rather see any form of methods with longer names.

[sveinse]

That's fine for me, I understand that the proposal meddles with principle architectural decisions.

If canopen decides to include asyncio support, the regular synchronous mechanism of using attr getters/setters cannot be used. Hence, the effect of this choice will be that the regular sync use (e.g. v = param.phy) will have be changed to other methods for async use (e.g. v = await param.aget_phy()). Other dual async/regular libraries tries to keep consistency between the variants and this was my suggestion to bridge this gap. That's all.