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

Schema: Move node-selection's href and user-content into choice #307

Merged
merged 135 commits into from
Jan 18, 2019

Conversation

AirQuick
Copy link
Member

@AirQuick AirQuick commented Aug 26, 2018

This pull request derives from #306. So needs to be handled after that.

Obsoletes #171.


In the current schema, node-selection makes both href and user-content optional:

xspec/src/schemas/xspec.rnc

Lines 190 to 195 in 5c87ec0

node-selection =
## The document URI (location) of the imported document.
attribute href { xs:anyURI }?,
## The XPath expression to access the selected node.
attribute select { xpath }?,
user-content

user-content = mixed { user-element* }

So it allows these elements

<x:context href="uri">content</x:context>
<x:param href="uri" name="foo">content</x:param>
<x:expect href="uri" label="foo">content</x:expect>

which don't make sense.

As for x:context, the scenario is terminated:

<xsl:if test="$context/@href and ($context/node() except $context/x:param)">
<xsl:message terminate="yes">
<xsl:text>ERROR in scenario "</xsl:text>
<xsl:value-of select="x:label(.)" />
<xsl:text>": can't set the context document using both the href</xsl:text>
<xsl:text> attribute and the content of &lt;context&gt;</xsl:text>
</xsl:message>
</xsl:if>

As for x:param and x:expect, content is ignored in favor of @href:

<variable name="{$var}-doc" as="document-node()">
<xsl:choose>
<xsl:when test="@href">
<xsl:attribute name="select">
<xsl:text>doc('</xsl:text>
<xsl:value-of select="resolve-uri(@href, base-uri(.))" />
<xsl:text>')</xsl:text>
</xsl:attribute>
</xsl:when>
<xsl:otherwise>
<document>
<xsl:apply-templates mode="test:create-xslt-generator" />
</document>
</xsl:otherwise>
</xsl:choose>
</variable>

This pull request updates the schema to reflect it. The elements above are no longer allowed by the updated schema.

This pull request consists of three commits:

  • 3c01377 verifies that the implementation does select href over user-content.
  • f654c1b updates the schema.
  • d2fc959 verifies that the schema does detect violation.

…c-rule

# Conflicts:
#	build.xml
#	test/saxon-custom-options/config.xml
#	test/saxon-custom-options/test.xspec
#	test/xspec-bat.cmd
#	test/xspec.bats
…tion_cumulative

# Conflicts:
#	build.xml
#	test/saxon-custom-options/config.xml
#	test/saxon-custom-options/test.xspec
#	test/xspec-bat.cmd
#	test/xspec.bats
…k/xspec into valid-xspec-rule

# Conflicts:
#	test/xspec-bat.cmd
#	test/xspec.bats
…c-rule

# Conflicts:
#	build.xml
#	test/end-to-end/generate-expected.cmd
#	test/end-to-end/generate-expected.sh
#	test/end-to-end/processor/html/_normalizer.xsl
#	test/end-to-end/run-e2e-tests.cmd
#	test/end-to-end/run-e2e-tests.sh
#	test/run-xspec-tests-ant.cmd
#	test/run-xspec-tests-ant.sh
…tion_cumulative

# Conflicts:
#	build.xml
#	test/end-to-end/generate-expected.cmd
#	test/end-to-end/generate-expected.sh
#	test/end-to-end/processor/html/_normalizer.xsl
#	test/end-to-end/run-e2e-tests.cmd
#	test/end-to-end/run-e2e-tests.sh
#	test/run-xspec-tests-ant.cmd
#	test/run-xspec-tests-ant.sh
…c-rule

# Conflicts:
#	test/xspec-bat.cmd
#	test/xspec.bats
…tion_cumulative

# Conflicts:
#	test/xspec-bat.cmd
#	test/xspec.bats
…c-rule

# Conflicts:
#	test/xspec-bat.cmd
#	test/xspec.bats
@AirQuick AirQuick added this to the v1.2.0 milestone Dec 24, 2018
…tion_cumulative

# Conflicts:
#	test/run-bats.cmd
#	test/run-bats.sh
#	test/win-bats/collection.xml
#	test/xspec.bats
…tion_cumulative

# Conflicts:
#	bin/xspec.bat
#	test/win-bats/collection.xml
#	test/xspec.bats
@AirQuick
Copy link
Member Author

Although this pull request contains a lot of commits for historical reasons, all that needs to be reviewed in the end is a set of commits mentioned in the initial comment:

  • 3c01377 verifies that the implementation does select href over user-content.
  • f654c1b updates the schema.
  • d2fc959 verifies that the schema does detect violation.

@AirQuick AirQuick requested a review from galtm December 31, 2018 07:10
Copy link
Member

@galtm galtm left a comment

Choose a reason for hiding this comment

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

Commits 3c01377 and f654c1b look good to me. (I had one minor line-specific comment that you can take or leave.) I need a little more time to review d2fc959 so I can become familiar with those files.

@AirQuick
Copy link
Member Author

AirQuick commented Jan 2, 2019

Thanks @galtm for the review. It's very helpful.

If you're interested, test/xspec.bats is Bats for *nix. Its syntax is written in its doc ("Writing tests" section).

So, in the commit, the test runs Jing to validate test/xspec-node-selection.xspec using src/schemas/xspec.rnc schema. And then the test verifies that Jing outputs exactly 5 violation lines and that every line contains -child-not-allowed. The test result on Travis is here.

test/win-bats/collection.xml is for Windows. Just a mimic of Bats. Its result on AppVeyor is here.

If you're not interested in d2fc959, feel free to review it just shallowly.

@galtm
Copy link
Member

galtm commented Jan 3, 2019

If you're interested,

Thanks for the background information, @AirQuick. You saved me some time. The tests look good.

Copy link
Member

@galtm galtm left a comment

Choose a reason for hiding this comment

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

Reviewed the three commits, and they look fine.

@AirQuick AirQuick merged commit 86151df into xspec:master Jan 18, 2019
@AirQuick AirQuick deleted the node-selection_cumulative branch January 18, 2019 14:51
@AirQuick
Copy link
Member Author

@cirulls
Merged this PR with the help from @galtm.
This is labeled as enhancement because it improves user experience. But some may call it a bug fix in that the schema didn't reflect the design correctly.

@cirulls
Copy link
Member

cirulls commented Jan 18, 2019

Thanks. I recorded it in the release notes as enhancement/new feature.

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

Successfully merging this pull request may close these issues.

None yet

3 participants