ENH: Add parser for Siemens "ASCCONV" text format #896
Conversation
Codecov Report
@@ Coverage Diff @@
## master #896 +/- ##
==========================================
+ Coverage 91.70% 91.71% +0.01%
==========================================
Files 96 98 +2
Lines 12311 12438 +127
Branches 2173 2191 +18
==========================================
+ Hits 11290 11408 +118
- Misses 684 688 +4
- Partials 337 342 +5
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
This looks quite reasonable to me. The code is well-exercised, except for error cases. Would it be possible to add a malformed file that hits these?
@matthew-brett Since this includes your contributions, would you care to have a look through?
|
Ok for me to add the fixes from my comments, and merge? |
|
Sounds good to me. @moloney, any objections? |
There was a problem hiding this comment.
tiny nitpick - this looks great, thanks @moloney and @matthew-brett!
Co-authored-by: Mathias Goncalves <[email protected]>
Co-authored-by: Mathias Goncalves <[email protected]>
|
Thanks - nitpicks applied! |
| if isinstance(target, ast.Attribute): | ||
| atoms.append(Atom(target, prev_target_type, target.attr)) | ||
| target = target.value | ||
| prev_target_type = OrderedDict |
There was a problem hiding this comment.
| prev_target_type = OrderedDict | |
| prev_target_type = dict |
| prot_dict : OrderedDict | ||
| Meta data pulled from the ASCCONV section. | ||
| attrs : OrderedDict |
There was a problem hiding this comment.
| prot_dict : OrderedDict | |
| Meta data pulled from the ASCCONV section. | |
| attrs : OrderedDict | |
| prot_dict : dict | |
| Meta data pulled from the ASCCONV section. | |
| attrs : dict |
| A line of the ASCCONV section could not be parsed. | ||
| ''' | ||
| attrs, content = ASCCONV_RE.match(ascconv_str).groups() | ||
| attrs = OrderedDict((tuple(x.split('=')) for x in attrs.split())) |
There was a problem hiding this comment.
| attrs = OrderedDict((tuple(x.split('=')) for x in attrs.split())) | |
| attrs = dict(x.split('=') for x in attrs.split()) |
| # Use Python's own parser to parse modified ASCCONV assignments | ||
| tree = ast.parse(content) | ||
|
|
||
| prot_dict = OrderedDict() |
There was a problem hiding this comment.
| prot_dict = OrderedDict() | |
| prot_dict = dict() |
| with open(ASCCONV_INPUT, 'rt') as fobj: | ||
| contents = fobj.read() | ||
| ascconv_dict, attrs = ascconv.parse_ascconv(contents, str_delim='""') | ||
| assert attrs == OrderedDict() |
There was a problem hiding this comment.
| assert attrs == OrderedDict() | |
| assert attrs == {} |
@matthew-brett Just saw this (again, probably...). Feel free to patch if you have time. Otherwise I can. |
|
I can do the changes. I think we'll need OrderedDict unless we are dropping Python 3.6. Is that imminent? |
|
No, we only just dropped 3.5. |
|
On further investigation, dicts are insertion-ordered in CPython as of CPython 3.6, and guaranteed to be so ordered from Python 3.7. PyPy actually had this before CPython, so I believe Python dicts are insertion ordered in PyPy 3.6. So our only worry for 3.6 would be some implementation other than CPython or PyPy, that has chosen some other algorithm for dicts, or kept the old CPython one for some reason. That seems very unlikely. So I think we can reasonably drop the requirement for OrderedDict soon. But - maybe not with this PR. Maybe that should be a separate change. What do y'all think? |
|
I put some more edits from the comments here into #935 Sorry - I should have applied the new commits here, but I forgot how to do that. |
|
Hi Matthew. #896 looks identical to this. Did you push all of your commits? |
|
Oops - done.
|
This is a clean PR after I screwed up the merge in #418
This includes Matthew Brett's hard work improving the parser (from his 'ascconv-refactor' branch).