Get rid of unique ids in SVG files. #14
Conversation
This commit makes the following assumptions: - SVG unique ids add a significant amount of noise to "git diff" outputs - SVG unique ids are only necessary when embedding multiple SVG files inside the same HTML page - even then, it was apparently necessary to further suffix style names - SVG snapshots are embedded only in the HTML report - SVG unique ids are kinda hard to control due to the "distance" between pytest and the function that actually generates them: 1. pytest_textual_snapshot.snap_compare.<locals>.compare() 2. textual._doc.take_svg_screenshot() 3. App.export_screenshot() 4. rich.Console.export_svg(... unique_id=None) This change introduces: - normalization of persisted SVG snapshots by stripping their ids; - individualization of embedded SVG pictures by randomizing their ids. Consequently: - this change does not require to regenerate existing snapshots; - future snapshots will not contain unique ids; - snapshots with ids can coexist with id-less snapshots; - conflict between embedded SVG styles is extremely unlikely despite the removal of style name suffixes.
| def _read_snapshot_data_from_location(self, *args, **kwargs) -> Optional["SerializableData"]: | ||
| """Normalize SVG data right after they are loaded from persistent storage.""" | ||
| data = super()._read_snapshot_data_from_location(*args, **kwargs) | ||
| if data is not None: | ||
| data = normalize_svg(data) | ||
| return data |
There was a problem hiding this comment.
I'm not sure about this. Subclassing a private method from a non-pinned 3rd party library is likely to eventually break if the method is removed or renamed.
There was a problem hiding this comment.
I agree, but I simply obeyed this:
https://github.com/syrupy-project/syrupy/blob/ef8189c68460593fead9c484405b755e272c8cca/src/syrupy/extensions/base.py#L140C10-L140C43
SnapshotCollectionStorage.read_snapshot(): This method is final, do not override. You can override
_read_snapshot_data_from_locationin a subclass to change behaviour.
There was a problem hiding this comment.
@darrenburns So, what do you think?
By the way, we could pin syrupy as an extra security measure.
There was a problem hiding this comment.
Sorry!
I think it makes sense to pin syrupy and merge this. Although perhaps longer term it'd make sense to have an option in Textual to write SVGs without the IDs rather than stripping them out.
There was a problem hiding this comment.
Sorry!
No problem, the week before the 1.0.0 release must have been quite busy.
I think it makes sense to pin syrupy and merge this.
Although perhaps longer term it'd make sense to have an option in Textual to write SVGs without the IDs rather than stripping them out.
Stricto sensu, that option already exists... but it cannot be leveraged from pytest-textual-snapshot.
As part of my personal reflections following this PR, I think the ideal situation is to introduce a specialized, diff-friendly format that represents a TUI screenshot and can be turned into an SVG picture. That new format would be versioned into Git instead of the SVG pictures whereas the SVG pictures would still appear in the HTML report. I started imagining a simple text format based on layers (background colors, text, foreground colors) but quickly noticed it lacks an elegant way to embed text formatting like bold, underline, etc. -- should this become a fourth layer or should it be shoehorned into the other layers? In the end, this endeavour is a significant amount of work for a rather small improvement, at a time when Textual users expect improvements in other areas (e.g. more widgets). So... yeah, definitely a longer term idea.
|
I've set a reminder to merge this on Monday. |
2 participants
This commit makes the following assumptions:
This change introduces:
Consequently: