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

Clarify allowed-values constraints #413

Conversation

aj-stein-nist
Copy link
Collaborator

@aj-stein-nist aj-stein-nist commented Aug 18, 2023

Committer Notes

Closes #411.

All Submissions:

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?
  • Have you squashed any non-relevant commits and commit messages? [instructions]
  • Do all automated CI/CD checks pass?

Changes to Core Features:

  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your core changes, as applicable?
  • Have you included examples of how to use your new feature(s)?
  • Have you updated all website](https://pages.nist.gov/metaschema) and readme documentation affected by the changes you made? Changes to the website can be made in the website/content directory of your branch.

@aj-stein-nist aj-stein-nist force-pushed the 411-clarify-specification-for-processing-combined-allowed-value-constraints branch from 430be3d to 4abfaef Compare August 18, 2023 16:23
@aj-stein-nist aj-stein-nist changed the title Change XSLT JSON Schema gen to allow . Clarify allowed-values constraints Aug 18, 2023
@aj-stein-nist aj-stein-nist changed the title Clarify allowed-values constraints [WIP] Clarify allowed-values constraints Aug 18, 2023
@aj-stein-nist aj-stein-nist self-assigned this Aug 18, 2023
@aj-stein-nist aj-stein-nist marked this pull request as ready for review August 18, 2023 19:00
@aj-stein-nist
Copy link
Collaborator Author

@david-waltermire-nist this is still WIP but I would like your preliminary thoughts and know if you and others agree with the direction this is headed in before finalizing.

@aj-stein-nist aj-stein-nist force-pushed the 411-clarify-specification-for-processing-combined-allowed-value-constraints branch from 091bd03 to 3e2a2a6 Compare August 31, 2023 16:28
@aj-stein-nist aj-stein-nist force-pushed the 411-clarify-specification-for-processing-combined-allowed-value-constraints branch from 581379c to 8e3d742 Compare August 31, 2023 17:03
wendellpiez
wendellpiez previously approved these changes Sep 1, 2023
Copy link
Collaborator

@wendellpiez wendellpiez left a comment

Choose a reason for hiding this comment

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

Nicely done! I have some remarks for discussion, no blockers.

website/content/specification/syntax/constraints.md Outdated Show resolved Hide resolved
website/content/specification/syntax/constraints.md Outdated Show resolved Hide resolved
website/content/specification/syntax/constraints.md Outdated Show resolved Hide resolved
website/content/specification/syntax/constraints.md Outdated Show resolved Hide resolved
website/content/specification/syntax/constraints.md Outdated Show resolved Hide resolved
website/content/specification/syntax/constraints.md Outdated Show resolved Hide resolved
website/content/specification/syntax/constraints.md Outdated Show resolved Hide resolved
website/content/specification/syntax/constraints.md Outdated Show resolved Hide resolved
@aj-stein-nist
Copy link
Collaborator Author

aj-stein-nist commented Sep 22, 2023

Per discussion with @david-waltermire-nist I am tardy in some of the edits I said I would make prior and some other recommendations in an enhanced TODO list below.

Outstanding items:

  • Defining the behavior around evaluating the target
  • A framework about how to handle a fatal versus non-fatal error
  • Finish what I had not pushed up from travel laptop, explaining target in all contexts (top level heading, not in allowed-values for assemblies/fields/flags sub-headings):
    • level
    • id
    • target

@aj-stein-nist aj-stein-nist force-pushed the 411-clarify-specification-for-processing-combined-allowed-value-constraints branch from cc2a34d to 5d49588 Compare October 16, 2023 17:43
@aj-stein-nist aj-stein-nist force-pushed the 411-clarify-specification-for-processing-combined-allowed-value-constraints branch from 0d95aae to ba2e754 Compare October 23, 2023 19:42
aj-stein-nist and others added 2 commits November 17, 2023 10:18
As part of usnistgov#411, use IETF BCP-14 languages as used
with other parts of the specification. Change the wording to address how
implementers would follow constraint directives in their processor, and
align example to be like computer model themes with tutorials and other
documentation.
aj-stein-nist and others added 9 commits November 17, 2023 10:18
Flesh out earlier draft with detailed coverage of allowed-values
constraints and semantics. Define `@allow-other` and `@extension`
attributes.

Co-authored-by: David Waltermire <[email protected]>
@aj-stein-nist aj-stein-nist force-pushed the 411-clarify-specification-for-processing-combined-allowed-value-constraints branch from ba2e754 to f22ea1b Compare November 17, 2023 15:19
@david-waltermire david-waltermire changed the title [WIP] Clarify allowed-values constraints Clarify allowed-values constraints Nov 17, 2023
@aj-stein-nist
Copy link
Collaborator Author

@david-waltermire-nist, re the checklist do you want me to squash commits now or after team and community review?

wendellpiez
wendellpiez previously approved these changes Dec 4, 2023
Copy link
Collaborator

@wendellpiez wendellpiez left a comment

Choose a reason for hiding this comment

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

Couple of longer-term questions/concerns, nothing blocking.

Nice work here!

website/content/specification/syntax/constraints.md Outdated Show resolved Hide resolved
Fix typo pointed out by @wendellpiez in PR review, ignore->ignored, in the section about the target attribute.

Co-authored-by: Wendell Piez <[email protected]>
@aj-stein-nist
Copy link
Collaborator Author

Also @david-waltermire-nist per the checklist let me know if you want me to squash the commits from the heavy editing phase before we finalize this and close it out.

@wendellpiez
Copy link
Collaborator

I have usnistgov/metaschema-xslt#91 on this.

The current implementation of the InspectorXSLT Metaschema transpiler works only when @target paths conform to the 'pattern subset' of XPath defined for XSLT. https://www.w3.org/TR/xslt-30/#pattern-syntax

FWIW, the reason the code has this limitation at all is because it started as a mapping of Metaschema rules to Schematron, in which rule/@pattern has this limitation.

Copy link
Collaborator

@david-waltermire david-waltermire left a comment

Choose a reason for hiding this comment

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

Looks good. I am going to merge this since it has been well reviewed.

@david-waltermire david-waltermire linked an issue Dec 29, 2023 that may be closed by this pull request
5 tasks
@david-waltermire david-waltermire merged commit 22a9594 into usnistgov:develop Dec 29, 2023
3 checks passed
david-waltermire added a commit that referenced this pull request Jan 4, 2024
* Update allowed-values overview. As part of #411, use IETF BCP-14 languages as used
with other parts of the specification. Changed the wording to address how
implementers would follow constraint directives in their processor, and
align example to be like computer model themes with tutorials and other
documentation.
* Corrected poorly worded assembly constraint explanation.
* Fleshed out earlier draft with detailed coverage of allowed-values
constraints and semantics. Define `@allow-other` and `@extension`
attributes.
* Add `@id`, `@level`, and `@target` for #411.
* Reorganization of common constraint data section and intro.
---------

Co-authored-by: David Waltermire <[email protected]>
Co-authored-by: Wendell Piez <[email protected]>
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

Successfully merging this pull request may close these issues.

Clarify specification for processing combined allowed-value constraints
3 participants