-
Notifications
You must be signed in to change notification settings - Fork 227
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
Embed YAML data into PNG #234
base: dev
Are you sure you want to change the base?
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.
Thanks for your explanation in #233.
I will play around with and think about this feature a bit later.
For now, I'm leaving some comments on the current implementation.
I will come back to this PR once I have time.
src/wireviz/wireviz.py
Outdated
with open_file_read(args.prepend_file) as fh: | ||
prepend = fh.read() | ||
yaml_input = prepend + yaml_input | ||
if ".png" in args.input_file: |
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.
Please consider using Pathlib
's .suffix
attribute for checking the file extension.
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.
Done
src/wireviz/wireviz.py
Outdated
yaml_input = prepend + yaml_input | ||
if ".png" in args.input_file: | ||
yaml_input = read_yaml_from_png(args.input_file.replace('.png','')) | ||
with open(args.input_file.replace('.png','_out.yaml'),'w') as fh: |
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.
Consider using wv_helper.py
's open_file_read()
and open_file_write()
functions for file access to ensure UTF-8 encoding
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.
Done, thanks!
I changed the |
A thought with regards to the implementation it might be worth either putting this capability behind a command line option, adding a command line option to not add the yaml data into the png (depending on the preference for weither to enable by default or not) or otherwise having a way to get an ouptut without the input embedded. The reasons I can think of for not wanting to add the input into the png are.
Another soloution for both of these would be to trim down/regenerate the yaml added to the png to only that which is needed to generate the diagram but that feels like it could be quite a complicated thing to do. |
@Tyler-Ward, really good points, I hadn't thought of those aspects.
A good thing to check, I just ran it with a 4KB prepend & 3.2KB diagram file. The ending png is 213KB with the data, or 211KB without the data. The size difference I'm assuming is the compressed state of the yaml, which results from the line:
I think that is a good point, I'll leave that up to @formatc1702 for comment. I can certainly implement that, but the questions would be:
|
The other formats, SVG & HTML also could incorporate these but those implementations are a bit more straightforward because they could just be added as comments if desired, and would be able to be just copy/pasted instead of needing to be read by the library. But since those will not have a compression option built-in, the file size will more likely be 1:1 increase by adding the data. |
Adjusted some typehints that linting was complaining about.
Update: Rebased on the latest dev version for easier merging |
@Tyler-Ward I just added a command line argument to 'conceal-input' so that this feature can be turned off. |
Is a solution for #233.