Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Confusing Variable class names used for different purposes #363

Closed
sveinse opened this issue Apr 6, 2023 · 1 comment
Closed

Confusing Variable class names used for different purposes #363

sveinse opened this issue Apr 6, 2023 · 1 comment

Comments

@sveinse
Copy link
Contributor

sveinse commented Apr 6, 2023

After spending quite many hours in the codebase, there is one thing I find really confusing and time consuming: Some classes are named the same thing, but with different functions. Applies to Variable and partly to Array and Record.

    variable.Variable
    pdo.base.Variable  # Inherited from variable.Variable
    sdo.base.Variable  # Inherited from variable.Variable
    objectdictionary.Variable

I understand the functions for all of these. and I get it if the intention was to make a duck-type like class, but they're really not all compatible. It does not help that type checkers and linters often displays the base name of the class, not the full path. If the type checker report an error where it expects Variable, a small investigation is required to figure out which of these classes is the one that should be used.

I would like to suggest to change the class names into unique names as follows:

    class Variable:  # in variable.py
    class PdoVariable(Variable):
    class SdoVariable(Variable):
    class ODVariable:

Likewise for Array into class SdoArray and class ODArray, and Record into class SdoRecord and class ODRecord. I don't intend any changes to the methods and the class inheritance. If we need to keep the old names intact for compatibility, we can do so as aliases in the respective files. I can spin up a PR if there's interest for this.

@acolomb
Copy link
Collaborator

acolomb commented Apr 7, 2023

I wouldn't mind as long as the aliases are still available to not break other code.

sveinse added a commit to sveinse/canopen-asyncio that referenced this issue Apr 7, 2023
To create better distinction between the different Variable, Record and Array types used for different purposes. Helps development, as IDE/linters often only display base name of class.
christiansandberg pushed a commit that referenced this issue Sep 1, 2023
To create better distinction between the different Variable, Record and Array types used for different purposes. Helps development, as IDE/linters often only display base name of class.
@sveinse sveinse closed this as completed May 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants