[WIP] introduction of Arbitration_id class #296
Conversation
add conversion to compound integer
Codecov Report
@@ Coverage Diff @@
## development #296 +/- ##
===============================================
+ Coverage 37.81% 38.16% +0.35%
===============================================
Files 34 34
Lines 6820 6841 +21
Branches 1675 1669 -6
===============================================
+ Hits 2579 2611 +32
+ Misses 4023 4016 -7
+ Partials 218 214 -4
Continue to review full report at Codecov.
|
|
@altendky It's not necessary to look on every single source line which had to be changed... |
There was a problem hiding this comment.
I didn't check to see how exactly this compared with what I had before so for all I know my suggestions are really just correcting myself... If it would be easier I can just make a PR against this to show what I have in mind. That would force me to work through any conflicts that arise from my suggestions. :]
| compound_extended_mask = (1 << 31) | ||
|
|
||
| id = attr.ib(default=None) | ||
| extended = attr.ib(default=None) |
There was a problem hiding this comment.
| extended = attr.ib(default=None) | |
| extended = attr.ib(default=True) |
| extended = attr.ib(default=None) | ||
|
|
||
| def __attrs_post_init__(self): | ||
| if self.extended is None or self.extended: |
There was a problem hiding this comment.
| if self.extended is None or self.extended: | |
| if self.extended: |
There was a problem hiding this comment.
there was the requirement to get both arbitration ids - the extended and the basic one - with the "by_id" method. Therefore the "None" was. If you set extended to "None" and compare two arbitration ids you'll get a true for both - the extended and the basic version.
There was a problem hiding this comment.
Hmm... maybe by_id() should be return itertools.chain.from_iterable(by_id(id, extended) for type in (False, True)) or less niftily return by_id(id, False) + by_id(id, True). (aside from the recursion in both cases...). I'm really not sure. I'm just taking to simpler classes at this point. :| Do what you want and we'll see how we feel about it later.
| else: | ||
| return self.id | ||
|
|
||
| def __eq__(self, other): |
There was a problem hiding this comment.
I think we should be able to drop this after the above changes? The default attrs-provided __eq__ should work and if it doesn't we should probably try to look for a way to make it.
| id = attr.ib(default=None) | ||
| extended = attr.ib(default=None) | ||
|
|
||
| def __attrs_post_init__(self): |
There was a problem hiding this comment.
I'm tending away from writing __attrs_post_init__s anymore. I think this could be just a helper @classmethod that the from_compound_integer() calls. We should be able to construct any class we want rather than having to create it and then modify to get to any arbitrary set of attributes.
| raise ArbitrationIdOutOfRange('ID out of range') | ||
|
|
||
| @classmethod | ||
| def from_compound_integer(cls, i): |
There was a problem hiding this comment.
| def from_compound_integer(cls, i): | |
| def from_compound_integer(cls, i, extended=True): |
And associated changes inside the function.
There was a problem hiding this comment.
... the compound_integer already has the extendedinformation in bit 31 - so no need for the extended attribute. Or do I miss something?
There was a problem hiding this comment.
:[ oh right, it's compound... I'm really not on my game today, sorry.
src/canmatrix/canmatrix.py
Outdated
| @@ -574,10 +617,10 @@ class Frame(object): | |||
| """ | |||
|
|
|||
| name = attr.ib(default="") | |||
| id = attr.ib(type=int, default=0) | |||
| arbitration_id = attr.ib(converter=Arbitration_Id.from_compound_integer, default=0) | |||
There was a problem hiding this comment.
Do we need to be so verbose re: arbitration_id over id? It might be better, but I'm not sure there's likely much confusion with just id. Either way.
There was a problem hiding this comment.
Attention, id is a python keyword. It would lead to "shadowing" warning.
There was a problem hiding this comment.
As it presumably has been already. I can't say I like it but it only shadows in the class definition itself. Once you are outside the class definition there's no more shadowing since it's self.id. Since you mostly shouldn't use id() at all anyways... I think I'm ok with this. It's a hazard of mixing declarative with imperative like attrs does in this style usage.
| size = attr.ib(default=0) | ||
| transmitters = attr.ib(type=list, factory=list) | ||
| extended = attr.ib(type=bool, default=False) | ||
| # extended = attr.ib(type=bool, default=False) |
There was a problem hiding this comment.
Hmm, So we're changing from default=False to default=True effectively above. I'm not sure which it should be, or how much it matters.
Do delete this before it gets left hanging around.
|
OK, I'll merge - we can work on it later again... |
should fix #144