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

Make display-xml more technically correct #3678

Closed
default-kramer opened this issue Feb 6, 2021 · 6 comments
Closed

Make display-xml more technically correct #3678

default-kramer opened this issue Feb 6, 2021 · 6 comments

Comments

@default-kramer
Copy link
Contributor

The documentation for display-xml says "newlines and indentation make the output more readable, though less technically correct when whitespace is significant." For example, the xexpr '(a (b "c")) is displayed as

<a>
  <b>
    c
  </b>
</a>

I was able to make a change such that this will print like

<a>
  <b>c</b>
</a>

My change is pretty limited in scope. The contents of <b> are not indented because all of its children are pcdata. This means that elements with mixed content will remain technically incorrect. Despite this limited scope, I think this change is still useful. I use Racket as a faster way to hand-craft test XML files, none of which have featured mixed content so far. Being able to display those files with both indentation and correct whitespace is nice.

Should I create a PR for this change?

@jeapostrophe
Copy link
Collaborator

I think it's a good idea, although I think it should probably be a parameter and not the default for backwards compatibility. :'(

@sorawee
Copy link
Collaborator

sorawee commented Feb 7, 2021

Should there also be a list of tags parameter that controls whether the content inside specified tags should be indented? I experienced a similar problem in @greghendershott's custom XML operation in his Markdown package (see greghendershott/markdown#75). In that context, the type of tags dictates whether it's safe to indent (see https://developer.mozilla.org/en-US/docs/Web/HTML/Inline_elements)

@sorawee
Copy link
Collaborator

sorawee commented Feb 7, 2021

Hmm. Actually the bug that I experienced was slightly different. In that one, it's about how:

<p>
  Hello<label></label>.
</p>

and

<p>
  Hello
  <label></label>
  .
</p>

are different (the below one has an extra space between "Hello" and "."). So it's not "inside specified tag", but rather, the entire specified tag.

@default-kramer
Copy link
Contributor Author

@jeapostrophe Good point, I can see now that the current behavior might be exactly what some callers want. For example '(html (body (p "This is line 1." "This is line 2"))) works great with display-xml, even if it is depending on "technically incorrect" behavior.

@sorawee If we want to allow the caller to customize the behavior, I think the most powerful solution is a callback that accepts the current node and returns two values: 1) whether the current node should be indented, and 2) a new callback to be used for the child nodes. I'll try this out and maybe some other strategies.

@sorawee
Copy link
Collaborator

sorawee commented Mar 8, 2021

@default-kramer now that #3686 is merged, can this issue be closed? Or are there still any outstanding issues?

@default-kramer
Copy link
Contributor Author

@sorawee Oops, I forgot about this. Yes, it can be closed.

@sorawee sorawee closed this as completed Mar 8, 2021
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

No branches or pull requests

3 participants