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

Fix unnecessary unescaping #26

Merged
merged 1 commit into from
May 5, 2024

Conversation

Kiisu-Master
Copy link
Contributor

I removed the unnecessary unescaping.
It is already done by Godots xml parser and the addition of this leads to double unescaping.

I noticed the strip_edges also modifies content in unwanted ways but when i remove it, it breaks...

Copy link
Owner

@elenakrittik elenakrittik left a comment

Choose a reason for hiding this comment

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

Thanks, i didn't even think Godot may've done that by itself! Looking through the sources it indeed calls .xml_unescape() when parsing.

I noticed the strip_edges also modifies content in unwanted ways but when i remove it, it breaks...

Can you elaborate? The .strip_edges() calls were always there, in that commit i only removed .lstrip() and .rstrip() calls because .strip_edges() already stripped spaces, so these unwanted changes must've been there before the commit too?

@Kiisu-Master
Copy link
Contributor Author

Kiisu-Master commented Apr 13, 2024

I noticed the strip_edges also modifies content in unwanted ways but when i remove it, it breaks...

Can you elaborate? The .strip_edges() calls were always there, in that commit i only removed .lstrip() and .rstrip() calls because .strip_edges() already stripped spaces, so these unwanted changes must've been there before the commit too?

Yes the strip_edges problem was already there before. I only noticed it now because I was looking at your changes on the same line.

The problem is that when you have a node with content that has spaces, these spaces will get removed from the edges when parsing. This means the content is not the same after dumping and parsing.

I don't have ideas how to fix it though. Reading the comment, it seems like a workaround for a Godot xml parser problem?

@Kiisu-Master Kiisu-Master marked this pull request as ready for review April 13, 2024 15:57
@elenakrittik
Copy link
Owner

elenakrittik commented Apr 13, 2024

The problem is that when you have a node with content that has spaces, these spaces will get removed from the edges when parsing.

Yep, that's intentional (but can be reverted, of course). I did it because i don't think that with, e.g.:

<node>
  Hello!
</node>

..one would expected the content to be "\n Hello!\n". At the same time i recognize that this practically forbids you from having multiline content, but that's an intentional trade-off i made in my mind when i've written it. Personally, my rule of thumb is "single-line data => make it an attribute; multi-line data => make it a CDATA", but i'm always open to feedback and we can change that (under an option).

Reading the comment, it seems like a workaround for a Godot xml parser problem?

In this case, it's more of an "unwanted" thing than an "issue" (the _cleanup_double_blankets() function below does exist to workaround an issue, but i assume that's out of your concern?).

@elenakrittik elenakrittik added the pr: bug A PR that fixs a bug. label Apr 13, 2024
@elenakrittik
Copy link
Owner

I'm going to merge this and open a separate issue for the "blank space problem". Thanks!

@elenakrittik elenakrittik merged commit 5f45212 into elenakrittik:master May 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr: bug A PR that fixs a bug.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants