-
Notifications
You must be signed in to change notification settings - Fork 31
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
feat: support Text Value Templates (expand-text) #711
Conversation
@AirQuick , I've started reviewing this pull request. This enhancement looks very promising! I need more time (maybe a week or so) but just wanted you to know that my review is in progress. |
Thanks @galtm It's always great to have a review. This pull request is unlikely to conflict with the others. It's ok to leave this opened for weeks. |
# Conflicts: # src/compiler/generate-common-tests.xsl
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm most of the way through this pull request. I think this feature is useful, well designed, and well aligned with how XSLT TVTs work.
Question: Is x:label
supposed to allow @expand-text
and support TVTs?
@@ -136,13 +136,25 @@ | |||
<xsl:attribute name="separator" select="." /> | |||
</xsl:when> | |||
|
|||
<xsl:when test="(. instance of text()) and x:is-user-content(.)"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is x:is-user-content(.)
always true for text nodes in this template? Just asking so I understand. If it's not always true, though, can there be a test that exercises that situation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is
x:is-user-content(.)
always true for text nodes in this template?
You're right, in the current usage patterns.
test:create-node-generator
creates a node generator. Callers of this mode may expect the created generator to do either of these two:
- When executed, the generator always generates the source node exactly as it was.
- When executed, the generator generates the source XSpec user-content node as its author indended.
I think that ideally,
(A) those two cases should be distinguished clearly,
and
(B) test:create-node-generator
should be able to serve both cases.
(A) requires cooridation with the callers.
is-user-content(.)
here is an attempt to satisfy (B) to some extent. It's incomplete but yet better than nothing until (A) is satisified.
If it's not always true, though, can there be a test that exercises that situation?
As you pointed out, test:create-node-generator
should be coupled with tests. In fact, the whole generate-tests-helper.xsl
as a module file should be tested. #729 is a small step toward it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I finished looking at the files and don't have any further comments beyond what I posted earlier. Good feature, @AirQuick !
Thanks for the informative review, @galtm. I'll resume this pr after the release of v1.5.0 gets fixed. |
# Conflicts: # src/compiler/generate-common-tests.xsl
FYI, I am planning to take another look at this PR over the weekend. You've made a lot of changes since my initial code review, so it seems like time for a fresh look. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other than one correction in a label, I have no other comments. Everything looks good.
Your numerous test cases not only make the feature more solid, but they'll also come in handy for our future reference about how different situations are supposed to work.
Now merged. |
This pull request introduces a feature just like XSLT 3.0's Text Value Templates.
Enabling/disabling TVT is controlled by
@expand-text
(or@x:expand-text
in user-content).Example
<test>
inis expanded to
Commits
@expand-text
.x:*
elements, like XSLT's@expand-text
.@expand-text
on Oxygen.common-attributes
onx:text
.@expand-text
inx:gather-specs
mode@expand-text
to the compiled XSLT intest:create-node-generator
mode.Limitation
TODO
.Further work
Document the feature briefly in Wiki.