-
Notifications
You must be signed in to change notification settings - Fork 14
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Adding support for Unknown Extra block and appended data after Terminal block #28
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for driving this improvement. Most of my remarks are responses to your questions asked in the related issue. I did not realize all the difficulties so it is good you have pointed them out. Also, could you please add two files mentioned in the issue as new tests?
I modified the Unknown block to not use a property, since none of the other block does. Regarding text output, I modified the print_lnk_file function to fit the current coding style, but I'm not a fan of hard-coding keys for which there is different behaviour. I went down a very deep rabbit hole to make it all agnostic. It would make those hard-coded list items re-use their parent instead: |
If you were curious to see what difference the agnostic+yaml changes makes in the output (and the code 😨), I uploaded a new branch with it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, just two last remarks (also, please fix linting issues so that all the tests will pass).
Regarding you agnostic print approach, I like it. Let's discuss it in the follow up PR.
No description provided.