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

Sprint24 model enhancements #516

Merged

Conversation

wendellpiez
Copy link
Contributor

Committer Notes

Addressing #499 and #511.

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:

Copy link
Contributor

@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.

These changes look right to me. I'd like to have @brianrufgsa also review before accepting this PR.

@david-waltermire
Copy link
Contributor

@wendellpiez Please rebase this branch against master.

@wendellpiez
Copy link
Contributor Author

Now rebased against latest this a.m.

Copy link
Contributor

@brian-ruf brian-ruf left a comment

Choose a reason for hiding this comment

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

This appears to meet the minimum requirement of Issue #511 by adding the @id-ref attribute to the add assembly. It appears the @position attribute has also be constrained to "before", "after", "starting", "ending" (consistent with our documentation), and is still available in cooperation with the new @id-ref.

wendellpiez added a commit to wendellpiez/OSCAL that referenced this pull request Nov 22, 2019
@wendellpiez
Copy link
Contributor Author

@brianrufgsa can we confer on this? Not sure we are looking at the same thing. I see both add/@id-ref and add/@position with the enumerated values starting ending before and after. (I am looking at the metaschema and testing a sample against the generated XSD.)

@wendellpiez
Copy link
Contributor Author

Oh I see, @brianrufgsa there's a typo in your comment I think we're actually okay.

Of course now I have pushed again we need to review the PR again (sorry). heads-up @david-waltermire-nist I added a bit more to the schema docs.

@wendellpiez
Copy link
Contributor Author

This PR also addresses #499

brian-ruf
brian-ruf previously approved these changes Nov 26, 2019
Copy link
Contributor

@brian-ruf brian-ruf left a comment

Choose a reason for hiding this comment

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

See comment from my review performed on 11/21.

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.

3 participants