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.
|
Updated with lastest commits in master. |
| 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.
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.