Skip to content
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

Improve __repr__ and Viewbox #117

Merged
merged 15 commits into from
Aug 22, 2021
Merged

Improve __repr__ and Viewbox #117

merged 15 commits into from
Aug 22, 2021

Conversation

Sophist-UK
Copy link
Contributor

@Sophist-UK Sophist-UK commented Aug 13, 2021

  1. Viewbox init now handles various forms of arguments so it can recreate from a repr:
    • "x y w h"
    • x=x etc.
  2. Viewbox creates a correct better repr.
  3. SVGImage adds missing attributes, puts the href (which is potentially huge) last and (I hope this is correct) puts the href attribute in quotations so that the end is easily parseable.
  4. Various objects parse preserve_aspect_ratio as an attribute when the SVG attribute is preserveAspectRatio . Backwards compatibility is preserved.

1. Viewbox `init` now handles various forms of arguments so it can recreate from a repr:
    * "x y w h"
    * x=x etc.
2. Viewbox creates a correct better `repr`.
3. SVGImage adds missing attributes, puts the href (which is potentially huge) last and (I hope this is correct) puts the href attribute in quotations so that the end is easily parseable.
Since `**kwargs` are often converted using attribute name, we need to use `preserveAspectRatio` everywhere instead of `preserve_aspect_ratio`.

We also need to convert "none" to `None`.
@Sophist-UK
Copy link
Contributor Author

I believe this is ready for merging subject to review.

Copy link
Member

@tatarize tatarize left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

perserve_aspect_ratio cannot be changed without preserving backwards compatibility

This is a serious breaking change that is completely unneeded to tweaking viewbox to init a couple other ways.

svgelements/svgelements.py Outdated Show resolved Hide resolved
svgelements/svgelements.py Outdated Show resolved Hide resolved
svgelements/svgelements.py Show resolved Hide resolved
test/test_group.py Outdated Show resolved Hide resolved
@tatarize
Copy link
Member

tatarize commented Aug 19, 2021

  1. the internal values are always pythonic snake-case. The names as text strings use the exact svg values. See other examples, this is consistent throughout.
SVG_ATTR_PATTERN_CONTENT_UNITS = "patternContentUnits"
self.pattern_content_units = values[SVG_ATTR_PATTERN_CONTENT_UNITS]

SVG_ATTR_CLIP_UNIT_TYPE = "clipPathUnits"
self.unit_type = SVG_UNIT_TYPE_USERSPACEONUSE

@Sophist-UK
Copy link
Contributor Author

Apologies for breaking backward compatibility. Hopefully this is now OK.

svgelements/svgelements.py Show resolved Hide resolved
svgelements/svgelements.py Show resolved Hide resolved
@Sophist-UK
Copy link
Contributor Author

Sophist-UK commented Aug 21, 2021

Ok - hopefully this should be backwards compatible and addresses all concerns (which have also had unit tests added for).

Copy link
Member

@tatarize tatarize left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I'd prefer the snake case for the preserveAspectRatio it's pretty clear that that would also work. I think if I go into Issue 87 I'll actually opt to avoid all snake case and go pure SVG cases

@tatarize tatarize merged commit b207630 into meerk40t:master Aug 22, 2021
@Sophist-UK Sophist-UK deleted the patch-1 branch August 22, 2021 11:04
@Sophist-UK
Copy link
Contributor Author

Sophist-UK commented Aug 22, 2021

I thought that conformance to svg was more important.

It could potentially have been backwards incompatible, but:

  1. The repr for Viewbox was new, so no backwards to be incompatible with.
  2. So long as input was backwards compatible, repr seems to be something where backwards compatibility is less important.
  3. If we were going to make backwards compatibility inviolate, then any change would be backwardly incompatible.

Now you have brought my attention to there being other examples, I will put making similar "fixes" on my to-do list. 😉 But before you get too worried, getting to the top of the list seems unlikely.

@Sophist-UK
Copy link
Contributor Author

stroke-width is not handled properly.

some attribute name are hard-coded in repr rather than referring to the attribute contant.

but this is low priority so not aiming to address either of these any time soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants