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

Clarify expand-text and encoding #561

Closed
ndw opened this Issue Oct 15, 2018 · 16 comments

Comments

Projects
None yet
4 participants
@ndw
Contributor

ndw commented Oct 15, 2018

It's an error if expand-text is true and an encoding is specified. But expand-text defaults to true. Perhaps we should say that it defaults to true unless there's an encoding specified. But that's kind of messy.

@eriksiegel

This comment has been minimized.

Contributor

eriksiegel commented Oct 16, 2018

IMHO better to stay consistent and not get messy. So yes, if you want to specify an encoding you have to set @expand-text to false. Not a very big deal...

@ndw

This comment has been minimized.

Contributor

ndw commented Oct 16, 2018

Fair enough. Alternatively, we could just say that it's ignored. Not sure what's easier/least confusing.

@ndw

This comment has been minimized.

Contributor

ndw commented Oct 23, 2018

Opinions, folks?

@xml-project

This comment has been minimized.

Contributor

xml-project commented Oct 24, 2018

Sorry, need to do some research to see what we are talking about. I guess it is

err:XS0088
It is a static error if a text node designated as a text value template appears in a p:inline where an encoding is specified.

I think this will turn out to be a difficult case for developers and result in unnecessary errors because I have an @expand-text somewhere in my pipeline. Turn it off is no big deal as @eriksiegel said, but remembering that I have to turn of is. So my tendency would be @ndw 's suggestion: Be friendly and ignore @expand-text (whatever value it has).

@ndw

This comment has been minimized.

Contributor

ndw commented Oct 24, 2018

It's worse than that. The default value is true, so if you specify an encoding on an inline, you must always add an expand-text attribute to turn it off.

I think ignoring it when encoding is specified is harmless (base64 encoded text will never include curly braces anyway).

@xml-project

This comment has been minimized.

Contributor

xml-project commented Oct 24, 2018

@ndw +1

@xml-project

This comment has been minimized.

Contributor

xml-project commented Oct 24, 2018

Second thought: I think you are right, but then the whole point of "@expand-text" anywhere was useless: @expand-text="true" does not change anything (because it defaults to true) and @expand-text="false" is useless for the same reason.
If I did not miss anything I propose to revert "expand-text" any where.

@eriksiegel

This comment has been minimized.

Contributor

eriksiegel commented Oct 26, 2018

If I did not miss anything I propose to revert "expand-text" any where.

Sorry @xml-project , what do you mean?

@xml-project

This comment has been minimized.

Contributor

xml-project commented Oct 26, 2018

As far as I understand it, we have @expand-text anywhere, but it is useless, because @expand-text on p:inline always defaults to "true". Doesn't this mean, that any setting of @expand-text='false' on a parent is overwritten by p:inline?
What did I miss?

@gimsieke

This comment has been minimized.

Contributor

gimsieke commented Oct 26, 2018

“Defaults to true” should be interpreted as: If expand-text is not set by an ancestor or the current p:inline itself, it must be assumed to be true.

@xml-project

This comment has been minimized.

Contributor

xml-project commented Oct 26, 2018

“Defaults to true” should be interpreted as: If expand-text is not set by an ancestor or the current p:inline itself, it must be assumed to be true.

Yes, this was the behaviour I would like to see. But I lost track of the discussion on expand-text, so I understood Norm's comment wrong.

@ndw

This comment has been minimized.

Contributor

ndw commented Oct 26, 2018

I think it's spec'd that way now. There's no mention of expand-text in the description of p:inline, its value is inherited "down" the tree, and it's true if it's either explicitly true or of it is entirely unspecified.

So putting expand-text="false" on p:declare-step would turn it off for the whole pipeline (unless it was turned back on by some descendant, of course).

If that's not what it says, that's what I think it should say.

@xml-project

This comment has been minimized.

Contributor

xml-project commented Oct 26, 2018

@ndw +1
But, just because I am curious. What did you mean by:

It's worse than that. The default value is true, so if you specify an encoding on an inline, you must always add an expand-text attribute to turn it off.

@ndw

This comment has been minimized.

Contributor

ndw commented Oct 26, 2018

Ah, I see how that was misleading. The particular pipeline that I was writing had only one inline and that was encoded. So there was no reason to mention expand-text, but I still had to set it explicitly false. In a pipeline where there was no text to expand that seemed annoying.

Do we have consensus that:

  1. The spec currently describes the semantics we want
  2. We want to add a rule that says it's ignored if encoding is specified.
@gimsieke

This comment has been minimized.

Contributor

gimsieke commented Oct 26, 2018

yes to 1. (I just checked the current prose, “If it has the value ‘true’, or if no such attribute is present among ancestors, then the text and attribute nodes are value templates.”), yes to 2.

@xml-project

This comment has been minimized.

Contributor

xml-project commented Oct 26, 2018

ad 1: In @gimsieke I trust.
ad 2: Yes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment