Rename Variable, Record and Array (#363) #368
Conversation
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
left a comment
christiansandberg
left a comment
There was a problem hiding this comment.
Seems like a good improvement. As long as we keep backwards compatibility I see no problem.
|
Updated with lastest commits in master. |
acolomb
left a comment
acolomb
left a comment
There was a problem hiding this comment.
Looks good in principle, just a nit-pick about inconsistent uppercase variants.
|
|
||
|
|
||
| class Variable(variable.Variable): | ||
| class PdoVariable(variable.Variable): |
There was a problem hiding this comment.
This is pure bikeshedding, but why do you use CamelCase with the lowercase Pdo abbreviation here, while for the OD you use the all-caps OD spelling as in ODVariable? Should consistently use uppercase for abbreviations here.
There was a problem hiding this comment.
@acolomb you're right, this is inconsistent. I didn't make a very deliberate choice, rather trying to adopt the gist from the rest of canopen. After looking more closer at it, I see canopen is equally inconsistent, e.g. class PDO vs class PdoBase. Looking at all class names, the majority is CamelCase, with the notable exception of PDO, TPDO, RPDO and not used with an additional suffix.
To be consistent with naming in canopen, ODVariable should then be OdVariable perhaps?
There was a problem hiding this comment.
Yes, I guess that's an acceptable solution. Just don't introduce even more inconsistency ;-)
There was a problem hiding this comment.
Naming is probably inconsistent everywhere else anyway, so it's not super important. Not sure what's best for readability, but I'm nowadays leaning more towards keeping abbreviations upper-case even if it makes it harder to separate the words.
|
|
||
|
|
||
| class Record(Mapping): | ||
| class SdoRecord(Mapping): |
There was a problem hiding this comment.
Same here, SDORecord? Please just be consistent.
There was a problem hiding this comment.
With reference to comment above, SdoRecord is correct if SdoBase is correct I would argue.
3 participants
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. Discussed in issue #363
Unittests pass and generated sphinx documents have been skimmed over.