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

Fix wrong category for NXsnsevent application definition #1160

Merged
merged 1 commit into from
Jun 30, 2022
Merged

Fix wrong category for NXsnsevent application definition #1160

merged 1 commit into from
Jun 30, 2022

Conversation

PeterC-DLS
Copy link
Contributor

No description provided.

@PeterC-DLS PeterC-DLS added this to the NXDL 2022.06 milestone Jun 30, 2022
@PeterC-DLS PeterC-DLS changed the title Fix wrong category for application definition Fix wrong category for NXsnsevent application definition Jun 30, 2022
@PeterC-DLS PeterC-DLS added the contributions Add or modify a contributed definition label Jun 30, 2022
woutdenolf
woutdenolf reviewed Jun 30, 2022
View reviewed changes
Copy link
Contributor

@woutdenolf woutdenolf left a comment

Choose a reason for hiding this comment

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

No idea what this should be.

@woutdenolf woutdenolf self-requested a review June 30, 2022 20:57
woutdenolf
woutdenolf approved these changes Jun 30, 2022
View reviewed changes
Copy link
Contributor

@woutdenolf woutdenolf left a comment

Choose a reason for hiding this comment

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

Well I guess it's not that different from NXsnshisto. So LGTM.

@prjemian
Copy link
Contributor

Both NXsnshisto and NXsnsevent should be base classes.

@prjemian
Copy link
Contributor

But SNS has never asked for them to be ratified by the NIAC.

@PeterC-DLS
Copy link
Contributor Author

I guess we can deprecated them in next release...

@woutdenolf
Copy link
Contributor

Both NXsnshisto and NXsnsevent should be base classes.

So either undo #1161 and both remain base classes or merge this PR and both are application definitions.

@PeterC-DLS
Copy link
Contributor Author

PeterC-DLS commented Jun 30, 2022 •
edited
Loading

They are both application definitions, right? They both specify NXentry and set definition fields!

@prjemian
Copy link
Contributor

I guess we can deprecated them in next release...

That might be a good question for @peterfpeterson (one of the original authors) or @KedoKudo (the current SNS rep on the NIAC).

@prjemian
Copy link
Contributor

They are both application definitions, right? They both specify NXentry and set definition fields!

OK. I confess this was all I examined today:

(bluesky_2022_3) prjemian@zap:~/.../NeXus/definitions$ git grep category | grep NXsns
contributed_definitions/NXsnsevent.nxdl.xml:<definition type="group" name="NXsnsevent" category="base" extends="NXobject"
contributed_definitions/NXsnshisto.nxdl.xml:<definition type="group" name="NXsnshisto" category="base" extends="NXobject"

prjemian
prjemian reviewed Jun 30, 2022
View reviewed changes
Copy link
Contributor

@prjemian prjemian left a comment

Choose a reason for hiding this comment

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

same for NXsnshisto

@PeterC-DLS
Copy link
Contributor Author

@prjemian okay to merge then?

prjemian
prjemian approved these changes Jun 30, 2022
View reviewed changes
Copy link
Contributor

@prjemian prjemian left a comment

Choose a reason for hiding this comment

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

yes

@PeterC-DLS PeterC-DLS merged commit 63de01c into main Jun 30, 2022
@PeterC-DLS PeterC-DLS deleted the fix-snsevent-category branch June 30, 2022 21:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@prjemian prjemian prjemian approved these changes

@woutdenolf woutdenolf woutdenolf approved these changes

Assignees

@PeterC-DLS PeterC-DLS

Labels
code camp contributions Add or modify a contributed definition
Projects
None yet
Milestone
NXDL 2022.06
Development

Successfully merging this pull request may close these issues.
3 participants
@PeterC-DLS @prjemian @woutdenolf