T196 fix additional frame #197

Funth0mas · 2018-09-27T08:40:33Z

Extend --additionalFrameAttributes to support any attribute name. Add this feature to json export. Replace eval() by getattr().

codecov-io · 2018-09-27T09:26:20Z

Codecov Report

Merging #197 into development will increase coverage by 0.03%.
The diff coverage is 2.38%.

@@              Coverage Diff               @@
##           development    #197      +/-   ##
==============================================
+ Coverage        10.16%   10.2%   +0.03%     
==============================================
  Files               24      24              
  Lines             5694    5674      -20     
  Branches          1591    1596       +5     
==============================================
  Hits               579     579              
+ Misses            5069    5049      -20     
  Partials            46      46

Impacted Files	Coverage Δ
src/canmatrix/xls_common.py	`4.91% <0%> (ø)`	⬆️
src/canmatrix/cmcsv.py	`14.68% <0%> (+0.59%)`	⬆️
src/canmatrix/kcd.py	`1.19% <0%> (ø)`	⬆️
src/canmatrix/cmjson.py	`6.89% <0%> (-0.66%)`	⬇️
src/canmatrix/xls.py	`2.15% <0%> (+0.07%)`	⬆️
src/canmatrix/copy.py	`7.14% <0%> (ø)`	⬆️
src/canmatrix/xlsx.py	`2.09% <0%> (+0.06%)`	⬆️
src/canmatrix/sym.py	`33.25% <0%> (ø)`	⬆️
src/canmatrix/canmatrix.py	`29.44% <16.66%> (-0.08%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 013449e...f7d6336. Read the comment docs.

altendky · 2018-09-27T13:33:07Z

Yeah, eval() is... ugly. But, I'm not a huge fan of making dict keys available as attributes either. The cli effect could be achieved without that as well. I can't say I have a really strong argument, it mostly just seems less 'magical'. (and yes, I have worked with __getattr__ etc, so I don't mean magical like I am not familiar with it)

getattr(signal, item, signal.attributes.get(item, ""))

Or just add a getter.

NOTHING = object()
def get(self, item, default=NOTHING):
    item = getattr(self, item, self.attributes.get(item, default))

    if item is NOTHING:
        raise SomeException()

    return item

signal.get(item, "")

Funth0mas · 2018-09-27T19:46:13Z

Totally agree, your solution is better. I'll apply it.

There is still question if it make sense to have some "additional attributes" as instance variables and some in the attributes dictionary. It would make sense to have them all on a single place. But I didn't want to touch the existing code, because... I'm just a passerby.

altendky · 2018-09-27T19:56:19Z

If I remember a conversation with @ebroecker correctly, it would be nice to revisit these 'attributes' and try to figure out which are common and how to nestle the rest together happily. Basically, there is a dbc bias and with the extra knowledge of other formats we could probably do a better job picking regular attributes to have.

Thanks for seeing the mess and doing something about at least part of it. :] We appreciate the help.

ebroecker · 2018-09-27T19:56:47Z

@Funth0mas
having all attributes in one place is not that easy...
My intention was to have the "common" attributes direct in the frame (yes some are missing).
The attributes[]-Dictionary is more or less a "dbc-only" place to save all dbc-attributes. The problem of dbc is, that you can define any attribute in dbc.
Some attributes are not yet in the common list (#146)

altendky

Thanks for the follow up. I won't make a fuss about pulling this apart but keep in mind for the future that this would be easier to review as three separate PR's. The original, the formatting, and the comments. I prefer the modified formatting but I'll leave it to @ebroecker to decide. Cheers.

src/canmatrix/canmatrix.py

altendky · 2018-10-02T21:01:54Z

src/canmatrix/canmatrix.py

+        :param default: Default value if attribute doesn't exist.
+        :return any: Return the attribute value if found, else `default` or None
+        """
+        if hasattr(self, name):


Sorry to evolve my request here... I forgot this was an attrs class and I think this will be a touch better. It will avoid considering methods and such as attributes.

https://www.attrs.org/en/stable/api.html#attr.fields_dict

if name in attr.fields_dict(type(self)):

I'll note that this is technically a breaking change given the it will block access via this method to .attribute items that shadow the names of regular attributes. But, I don't think that bothers me.

src/canmatrix/canmatrix.py

altendky · 2018-10-02T21:10:41Z

src/canmatrix/cmjson.py

@@ -101,21 +101,21 @@ def dump(db, f, **options):
                              "signals": signals}
            frame_attributes = {}
            for frame_info in additionalFrameColums:  # Look for additional Frame Attributes
-                if hasattr(frame, frame_info):
-                    frame_attributes[frame_info] = getattr(frame, frame_info)
+                if frame.attribute(frame_info):  # does the attribute exist?


This doesn't check it's existence. If it exists but is None or anything else falsey it will be treated as not present. Here's the first thing that came to my mind, though it is a touch smelly to me. Maybe you can think of something nicer.

MISSING = object() value = frame.attribute(name=frame_info, default=MISSING) if value is not MISSING: frame_attributes[frame_info] = value

good point! As there is imho no reason to export None value, change to is not None

src/canmatrix/cmjson.py

Funth0mas · 2018-10-02T21:28:51Z

I ran the autoformating on the whole file by accident. If it's problem, I'll revert it line per line. Sorry for the inconvenience.
Mainly I removed the cryptic getattr and I propose to use the existing attribute() getter, but make the db parameter really optional. I added some docstrings (only in Frame) when trying to understand the code.

altendky · 2018-10-03T00:09:47Z

No big deal, I just tend to mention more rather than less. Thanks for the help improving the code.

…s().

Funth0mas · 2018-10-03T07:02:15Z

Ready for review again. Thanks!

ebroecker

Thank you for improving the code.
I'll accept it as is. The only change is the comment about "is_multiplexed" - maybe you could correct..

Thanks

src/canmatrix/canmatrix.py

src/canmatrix/cmcsv.py

altendky

Alrighty, so fix up the is_multiplex comment and the .detach() and I think it'll get merged. Thanks again for your patience and effort.

src/canmatrix/canmatrix.py

src/canmatrix/cmcsv.py

Funth0mas · 2018-10-04T07:10:34Z

The failed appveyor isn't my fault :-(

altendky · 2018-10-04T18:24:11Z

Agreed, we'll keep an eye on it and see if it was a temporary issue or not. (pypy3 is just broken, but yeah, pypy2 was working)

altendky · 2018-10-05T20:11:18Z

@Funth0mas are you referring to 3bdafa9? I generally do a git merge --no-ff development and get a merge commit. I'm not really sure quite why it was diffing the way it was... but I think I just fixed it. Fwiw, I did a git rebase -i HEAD~3 and deleted the squash commit then did a regular git merge development --no-ff and a force push to your branch (you let us have write access to just that branch when you submitted the PR). Yes, I did also push a backup branch to my repo first. :]

Anyways, if this looks right to you I'm ready to merge.

Funth0mas · 2018-10-05T20:34:45Z

@altendky funny thing is that I did almost the same except of using more complicated git reset and force pushed two minutes before you did :-) At least I understand the git a little bit more now.
I 'inspected' the diff once again and yes, it's the state I wanted to have. Thank you for your attention.

altendky · 2018-10-05T20:52:11Z

Thanks for all the code and your responsiveness.

Cheers,
-kyle

Funth0mas · 2018-10-06T06:34:50Z

My pleasure. BTW you're from Germany?
Regards

ebroecker · 2018-10-06T06:59:56Z

No, only I am from Germany, altendkey Not...

Tomas Novak added 3 commits September 27, 2018 10:11

Improve --additionalFrameInfo, add it to json. Refactor eval(). Add TC.

3409a78

Try to revert encoding changes in the test file.

747e0a6

Remove BOM

643a37e

Funth0mas closed this Sep 27, 2018

Fix RecursionError when deepcopying Frame

58bfa5e

Funth0mas reopened this Sep 27, 2018

Get frame attributes less cryptic. Add docstrings to Frame.

2cfb6d8

altendky requested changes Oct 2, 2018

View reviewed changes

Tomas Novak added 3 commits October 3, 2018 07:02

Revert autoformatting. Use named attr in all calls to Frame.attribute…

4118c94

…s().

Resolve PR issues - better attribute handling.

b67a751

Fix wrongly refactored frame.attribute() calls.

73fc969

ebroecker reviewed Oct 3, 2018

View reviewed changes

src/canmatrix/canmatrix.py Outdated Show resolved Hide resolved

src/canmatrix/canmatrix.py Outdated Show resolved Hide resolved

src/canmatrix/canmatrix.py Outdated Show resolved Hide resolved

src/canmatrix/cmcsv.py Show resolved Hide resolved

altendky requested changes Oct 3, 2018

View reviewed changes

src/canmatrix/canmatrix.py Outdated Show resolved Hide resolved

src/canmatrix/cmcsv.py Outdated Show resolved Hide resolved

Fix comment, specify exception and revert unneeded param name change.

08b6fe6

Funth0mas force-pushed the t196-fix_additional_frame branch from 768fecf to ac25589 Compare October 5, 2018 19:59

Tomas Novak and others added 2 commits October 5, 2018 16:04

Write csv with try/finally.

2e15b0e

Merge branch 'development' into t

f7d6336

altendky force-pushed the t196-fix_additional_frame branch from ac25589 to f7d6336 Compare October 5, 2018 20:07

altendky merged commit 30ae7e2 into ebroecker:development Oct 5, 2018

Funth0mas deleted the t196-fix_additional_frame branch October 6, 2018 07:35

Funth0mas mentioned this pull request Oct 30, 2018

[WIP] 'Simple' Frame tests and fixes #225

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

T196 fix additional frame #197

T196 fix additional frame #197

Funth0mas commented Sep 27, 2018

codecov-io commented Sep 27, 2018 •

edited

Loading

altendky commented Sep 27, 2018

Funth0mas commented Sep 27, 2018

altendky commented Sep 27, 2018

ebroecker commented Sep 27, 2018

altendky left a comment

altendky Oct 2, 2018

altendky Oct 2, 2018

Funth0mas Oct 3, 2018

Funth0mas commented Oct 2, 2018

altendky commented Oct 3, 2018

Funth0mas commented Oct 3, 2018

ebroecker left a comment

altendky left a comment

Funth0mas commented Oct 4, 2018

altendky commented Oct 4, 2018

altendky commented Oct 5, 2018

Funth0mas commented Oct 5, 2018

altendky commented Oct 5, 2018

Funth0mas commented Oct 6, 2018

ebroecker commented Oct 6, 2018

T196 fix additional frame #197

T196 fix additional frame #197

Conversation

Funth0mas commented Sep 27, 2018

codecov-io commented Sep 27, 2018 • edited Loading

Codecov Report

altendky commented Sep 27, 2018

Funth0mas commented Sep 27, 2018

altendky commented Sep 27, 2018

ebroecker commented Sep 27, 2018

altendky left a comment

Choose a reason for hiding this comment

altendky Oct 2, 2018

Choose a reason for hiding this comment

altendky Oct 2, 2018

Choose a reason for hiding this comment

Funth0mas Oct 3, 2018

Choose a reason for hiding this comment

Funth0mas commented Oct 2, 2018

altendky commented Oct 3, 2018

Funth0mas commented Oct 3, 2018

ebroecker left a comment

Choose a reason for hiding this comment

altendky left a comment

Choose a reason for hiding this comment

Funth0mas commented Oct 4, 2018

altendky commented Oct 4, 2018

altendky commented Oct 5, 2018

Funth0mas commented Oct 5, 2018

altendky commented Oct 5, 2018

Funth0mas commented Oct 6, 2018

ebroecker commented Oct 6, 2018

codecov-io commented Sep 27, 2018 •

edited

Loading