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.

Sign up for GitHub

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

Jump to bottom

add choice element to NXDL Schema #622

Merged
merged 7 commits into from
May 14, 2018
Merged

add choice element to NXDL Schema #622

merged 7 commits into from
May 14, 2018

Conversation

prjemian
Copy link
Contributor

fixes #619, pre-requisite for adding new geometry definitions to NXdetector

If we only want choice to apply to groups, this is ready to review and squash merge*.

If we also want to include fields (and explain why we want that), a bit more work will be needed.

After this is merged, this should be tagged Schema-3.4 (similar to Schema-3.3)

@prjemian prjemian added this to the NXDL 2018.5 milestone May 10, 2018
@prjemian prjemian self-assigned this May 10, 2018
matthew-d-jones
matthew-d-jones approved these changes May 11, 2018
View reviewed changes
Copy link
Member

@matthew-d-jones matthew-d-jones left a comment

Choose a reason for hiding this comment

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

Many thanks @prjemian, this achieves all we need for #601 and the supporting documentation looks clear to me 👍

zjttoefs
zjttoefs approved these changes May 11, 2018
View reviewed changes
Copy link
Contributor

@zjttoefs zjttoefs left a comment

Choose a reason for hiding this comment

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

Looking at the changes does answer most of my questions in #619
I suspect we may need to revisit this in the future, but for the moment it does all we need and I am impressed how quickly his was produced. Thanks @prjemian !

@prjemian
Copy link
Contributor Author

We could relax the requirement on the @name attribute for this PR or remove it altogether from the choice element. I don't see how we can enforce that (easily) through the NXDL Schema without defining a special groupGroup just for this in the nxdl.xsd file. Don't want to start that trend.

But, if this PR is good enough for today, these would be changes to be made upon request in the future. That's my vote.

@prjemian
Copy link
Contributor Author

After merging this, we should wait to tag a new Schema-3.4 version until #616 is complete.

@prjemian prjemian mentioned this pull request May 11, 2018
Merged
6 tasks
@zjttoefs
Copy link
Contributor

I'd be happy to merge. Let's give @mkoennecke some limited time to review, though.

mkoennecke
mkoennecke approved these changes May 14, 2018
View reviewed changes
Copy link
Contributor

@mkoennecke mkoennecke left a comment

Choose a reason for hiding this comment

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

I have looked at this an I am happy to merge this

@zjttoefs zjttoefs merged commit 693599a into master May 14, 2018
@zjttoefs zjttoefs deleted the choice_element_619 branch May 14, 2018 10:56
@prjemian prjemian mentioned this pull request May 15, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zjttoefs zjttoefs zjttoefs approved these changes

@mkoennecke mkoennecke mkoennecke approved these changes

@matthew-d-jones matthew-d-jones matthew-d-jones approved these changes

Assignees

@prjemian prjemian

Labels
enhancement high priority nxdl-syntax
Projects
None yet
Milestone
NXDL 2018.5
Development

Successfully merging this pull request may close these issues.

add choice element to NXDL
4 participants
@prjemian @zjttoefs @mkoennecke @matthew-d-jones