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

Don't inconsistently add XML declarations #1227

Merged
merged 1 commit into from
Dec 14, 2021

Conversation

isidentical
Copy link
Contributor

Resolves #1156

Copy link
Member

@jkbrzt jkbrzt left a comment

Choose a reason for hiding this comment

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

I think we should also include a test that verifies the behavior we’re correcting (i.e., that an unwanted XML declaration is indeed added by the library).


I just noticed that the parser also collapses empty tags:

$ echo '<a></a>' | http --offline pie.dev/post content-type:application/xml
<?xml version="1.0" encoding="utf-8"?>
<a/>

And I assume that might not be the last unwanted alteration. Not sure if we should spend too much time trying to fix this.

(I was against reintroducing the XML formatter but was assured that this one has none the issues of the original one, but it seems like we’re in a similar situation.)

httpie/output/formatters/xml.py Show resolved Hide resolved
httpie/output/formatters/xml.py Outdated Show resolved Hide resolved
httpie/output/formatters/xml.py Outdated Show resolved Hide resolved
httpie/output/formatters/xml.py Outdated Show resolved Hide resolved
httpie/output/formatters/xml.py Outdated Show resolved Hide resolved
@isidentical
Copy link
Contributor Author

isidentical commented Dec 6, 2021

I think we should also include a test that verifies the behaviour we’re correcting (i.e., that an unwanted XML declaration is indeed added by the library).

We already have such tests (we had tests that did not have a declaration in their raw version, but have it on their formatted. So I simply fixed them).

And I assume that might not be the last unwanted alteration. Not sure if we should spend too much time trying to fix this.

It is due to Python's parser is not really caring about roundtripability. I've looked around on PyPI, but I have not yet found an XML parser that claims to have a full-roundtripability feature. If that is what we are looking at, we could drop this or explicitly mention about the formatters are only support for semantical equivalence.

@codecov-commenter
Copy link

codecov-commenter commented Dec 6, 2021

Codecov Report

Merging #1227 (775716f) into master (4d7d6b6) will decrease coverage by 0.56%.
The diff coverage is 94.91%.

❗ Current head 775716f differs from pull request most recent head f2c0ca7. Consider uploading reports for the commit f2c0ca7 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1227      +/-   ##
==========================================
- Coverage   97.28%   96.72%   -0.57%     
==========================================
  Files          67       81      +14     
  Lines        4235     5398    +1163     
==========================================
+ Hits         4120     5221    +1101     
- Misses        115      177      +62     
Impacted Files Coverage Δ
tests/test_binary.py 100.00% <ø> (ø)
httpie/compat.py 31.11% <27.90%> (-68.89%) ⬇️
tests/conftest.py 77.14% <58.33%> (-9.82%) ⬇️
tests/test_ssl.py 91.01% <66.66%> (-3.93%) ⬇️
httpie/manager/__main__.py 82.35% <82.35%> (ø)
httpie/models.py 94.73% <83.33%> (-2.64%) ⬇️
httpie/output/formatters/colors.py 92.66% <88.88%> (-0.92%) ⬇️
httpie/manager/core.py 92.85% <92.85%> (ø)
httpie/manager/plugins.py 93.39% <93.39%> (ø)
httpie/cli/json_form.py 93.87% <93.87%> (ø)
... and 52 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update df58ec6...f2c0ca7. Read the comment docs.

@jkbrzt
Copy link
Member

jkbrzt commented Dec 8, 2021

I think we should also include a test that verifies the behavior we’re correcting (i.e., that an unwanted XML declaration is indeed added by the library).

We already have such tests (we had tests that did not have a declaration in their raw version, but have it on their formatted. So I simply fixed them).

What I mean is that we implicitly rely on the library auto-adding the declaration. For example, we assume there’ll always be lines[0].

Let’s add an explicit test case verifying this assumption still holds (something like assert pretty('<x/>') == '<?xml …?><x/>').

explicitly mention about the formatters are only support for semantical equivalence.

Let’s do that.

@isidentical
Copy link
Contributor Author

What I mean is that we implicitly rely on the library auto-adding the declaration. For example, we assume there’ll always be lines[0].

I see. I've changed it now to ensure there is at least one line and the first line is some sort of a declaration. If that is the case, we either get rid of it or replace it with the original version (if there are any).

Let’s do that.

Sure!

@isidentical isidentical force-pushed the ignore-xml-declaration branch 3 times, most recently from f2c0ca7 to 19cbfda Compare December 9, 2021 13:25
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.

Do not append missing XML declaration in formatted responses
3 participants