-
Notifications
You must be signed in to change notification settings - Fork 9
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
produce-xml-converter.xsl: XSpec tests and minor XSLT improvements #87
produce-xml-converter.xsl: XSpec tests and minor XSLT improvements #87
Conversation
<!-- ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ --> | ||
<!-- produce-json-converter.xsl reaches these templates by name | ||
instead of by match --> | ||
<x:scenario label="Tests for name=make-template template" pending="TODO"> |
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.
The TODO notations in this file are because I jotted down needs for future work. I anticipate that the work would be a separate pull request.
@@ -190,18 +199,19 @@ | |||
<xsl:call-template name="provide-namespace"/> | |||
<xsl:apply-templates select="." mode="make-key-flag"/> | |||
<xsl:apply-templates select="*" mode="make-pull"/> | |||
<xsl:variable name="keyname" select="@_key-name"/> |
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 moved this variable inside xsl:if
because that's the only place where the variable is used
<xsl:template priority="10" match="*[@scope = 'global'] | group[*/@scope = 'global']" mode="make-xml-match"> | ||
<xsl:template priority="10" match="*[@scope = 'global'] | group[*/@scope = 'global']" mode="make-xml-match" | ||
as="text()"> |
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.
Below this point, it looks like I changed a lot of lines. What I actually did was add @as
attributes and wrap the template output in xsl:value-of
. The reason for the latter is that when I considered the output data type of the template, it seemed simpler to consolidate multiple text nodes into a single text node.
|
||
<!-- A field without a GI is implicit in the XML; Metaschema prevents it from having flags --> | ||
<xsl:template priority="2" match="field[empty(@gi)][(.|value)/@as-type='markup-multiline']" mode="make-xml-pull"> | ||
<!-- This template takes precedence over the one that matches field, unqualified | ||
(even without priority=2 QUESTION: remove priority=2?) --> |
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.
Reviewers: Should I remove priority=2 or leave it as is?
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.
Hm, inclined to say leave as is, but maybe there's a reason to remove I am missing?
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 thought the priority wasn't necessary, but I'm fine with leaving it. I pushed a new commit that removes the parenthetical part of the comment.
<!-- QUESTION: The context is contrived and might not be the right way | ||
to exercise the code in the situation where $parent-match contains | ||
multiple match expressions. Does the context need fixing? --> |
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.
Reviewers: I wasn't sure about how to set up this scenario.
but no team members have recursive="true", so disambiguation | ||
of 1st and 3rd items is not needed. | ||
--> | ||
<!-- QUESTION: Is it OK that they're not distinct? --> |
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.
Question for reviewers...
XSpec tests: - XSpec tests for produce-xml-converter.xsl in isolation - Stub of tests for produce-xml-converter.xsl from the context of produce-json-converter.xsl - Helper XSLT for use in XSpec tests - Sample definition maps to provide testing contexts XSLT improvements: - Declare data types where known - Declare context item for named templates - Start XPath path from context instead of root, for testability
c2c751b
to
9744dc8
Compare
I rebased my commit off the develop branch and force-pushed, to refresh this pull request. The checks ran and all passed. However, I don't think the new XSpec tests that are in this pull request are being picked up in the CI/CD testing. |
@galtm it is possible the configuration for XSpec runtimes here is skipping yours - we will have to look. Most likely there is a line to add in a What is your current sense? -- after merging the earlier two outstanding PRs I wonder if we should look at this together. The CI/CD doesn't actually have to run yet (in my view we can come back to that) but just in case there is something else. Apart from that question, how close is this to ready? |
There are two comments that contain the word "reviewers" or "Reviewers" and that haven't been resolved yet. |
Sure, we can look at it together next time we talk. Or, if you are inclined to merge this even before the CI/CD runs the new tests, I'm OK with that. If the CI/CD somehow picks up the new tests after the PR is merged, great; if not, one of us can create a GitHub issue so we won't forget to come back to this later. |
Another option maybe we should consider is merging this PR into develop, but into a common branch where we can test further, prior to the develop merge. Indeed, such a staging branch might be a good place also to
And of course can work out remaining code/test issues there also. As for Thanks Amanda! |
I have no opinions about branching approaches, so whatever you want to do is fine with me. |
So ... I made branch Let's try pointing this PR there and seeing what trouble we get into. The merge branch is the same as Then on that branch I can address #117 before merging into |
I'm merging this into a merge branch where the work can be reviewed before pushing up to Also will work on #117 in that branch. |
* produce-xml-converter.xsl: XSpec tests and minor XSLT improvements (#87) * XSpec tests and minor XSLT improvements * JSON Schema generation improvements (#108) Refactoring submodules; XProc-based testing; XSpec testing of JSON Schema generation; improvements to JSON Schema generation - addressing #105 also #2007. * June 2024 Integration (#118) Integrating branches * June 2024 in-progress merge (#120) Includes updates to unit testing, Inspector XSLT, JSON Schema (#105) and others * More testing (#122) Mostly testing support - * Adding Makefile in compose phase to run XSpec - presently showing errors! * Extending and reorganizing testing, including removing temporary artifacts from git * Adding subpipeline mistakenly omitted from the last PR * New Makefile for testing/tinydata * Mending XSpecs testing Metaschema composition (regression protection); also a new tinydata sample. * Update src/testing/tinydata/tiny-data/terms-of-art.xml (AJ Stein) Co-authored-by: Wendell Piez <[email protected]> Co-authored-by: Amanda Galtman <[email protected]> Co-authored-by: A.J. Stein <[email protected]>
Committer Notes
Closes #86
XSpec tests:
XSLT improvements:
I ran the XSpec tests in Oxygen 26.0 and there are no failures.
All Submissions:
Changes to Core Features: