Add script to fix animations on windows - #383
Conversation
PeterJCLaw
left a comment
PeterJCLaw
left a comment
There was a problem hiding this comment.
Nice, thanks for putting this together. Some small suggestions inline to make this a bit more robust.
| output_data.append(f' <link rel="stylesheet" href="{css_name}">\n') | ||
| elif '<webots-view' in line: | ||
| match = re.search( | ||
| r'data-thumbnail=(.*?) .*data-scene=(.*?) .*data-animation=(.*?)>', line) |
There was a problem hiding this comment.
nit: please could we wrap this for consistency:
| r'data-thumbnail=(.*?) .*data-scene=(.*?) .*data-animation=(.*?)>', line) | |
| r'data-thumbnail=(.*?) .*data-scene=(.*?) .*data-animation=(.*?)>', | |
| line, | |
| ) |
| css_name = match[1].replace('\\', '/').split('/')[-1] | ||
| output_data.append(f' <link rel="stylesheet" href="{css_name}">\n') |
There was a problem hiding this comment.
Feels like it would be safer to do a find & replace on the adjusted text rather than fully rebuilding the line. That would avoid any issues with the underlying template changing (also below).
| jpg_name = match[1].replace('\\', '/').split('/')[-1] | ||
| x3d_name = match[2].replace('\\', '/').split('/')[-1] | ||
| json_name = match[3].replace('\\', '/').split('/')[-1] |
There was a problem hiding this comment.
Maybe worth asserting that the removed portion of these is all the same?
Perhaps like:
| jpg_name = match[1].replace('\\', '/').split('/')[-1] | |
| x3d_name = match[2].replace('\\', '/').split('/')[-1] | |
| json_name = match[3].replace('\\', '/').split('/')[-1] | |
| *jpg_dir, jpg_name = match[1].replace('\\', '/').split('/') | |
| *x3d_dir, x3d_name = match[2].replace('\\', '/').split('/') | |
| *json_dir, json_name = match[3].replace('\\', '/').split('/') | |
| assert jpg_dir == x3d_dir == json_dir |
| parser.add_argument('animation') | ||
| parser.add_argument('output') |
There was a problem hiding this comment.
For learnings: you might be interested in argparse.FileType, a class which can be passed to the type kwarg here. It handles opening the files as well as natively supporting using - for STDIN and or STDOUT.
| data = f.readlines() | ||
|
|
||
| output_data = [] | ||
| for line in data: |
There was a problem hiding this comment.
General style point for learning: I would encourage thinking about separating the logic which processes the content of the files from the logic which handles the file I/O. Doing so tends to make the code clearer as well as making the processing logic more easily reusable.
For this case I'd probably go with a separate function which operates on a single line at a time (and have a comment explicitly noting the assumption that the HTML elements of interest are each on a single line), keeping the loop in the same function as the IO.
2 participants
Makes absolute paths relative, assumes ancillary files are siblings of the html.
Fixes #378