-
Notifications
You must be signed in to change notification settings - Fork 56
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
Implement-NXobject-inheritance #1507
base: main
Are you sure you want to change the base?
Implement-NXobject-inheritance #1507
Conversation
When group classes that are valid in any base class are added to the NXobject base class, it is important to exclude NXroot to enforce the rule that only NXentry groups are permitted.
This is an anomalous entry that is the only example in the entire repository where the mechanism for defining classes is conflated with the contents of that class. Its presence here implies that the attribute is required, whereas it is completely redundant.
This is the subject of another PR and was mistakenly included here.
If you viewed this earlier, you might have seen changes to the NXroot NXDL file. This is the subject of another PR and the changes have now been removed to keep them separate. |
<group type="NXnote" minOccurs="0" /> | ||
<group type="NXparameters" minOccurs="0" /> | ||
<group type="NXtransformations" minOccurs="0" /> | ||
<group name="GROUPNAME_log" type="NXlog" nameType="partial"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should these be partial nameTypes optional too as their presence are not required if the matching instances are present.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think all partial names are automatically optional. The whole point is that they are an optional supplement any other field in the same group. If an application definition wants to require one, it needs to specify the name exactly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Being in a base class actually also means that it is optional (by default). As Ray said, if needed, it can be made required in an AppDef. Note that optionality is not connected to nameType that only specifies the naming convention rule for inheritance. As an AppDef can require INSTRUMENT with nameType=any, it can also require GROUPNAME_log with nameType=partial. That would only mean that NXlog instances of xxx_log, yyy_log, etc. all belong to this concept.
<group type="NXlog" minOccurs="0" /> | ||
<group type="NXnote" minOccurs="0" /> | ||
<group type="NXparameters" minOccurs="0" /> | ||
<group type="NXtransformations" minOccurs="0" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we also need a depends_on
attribute?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure why we would need to add it, since attributes are usually optional. Even the NXtransformations class appears in many application definitions without one.
This implements the proposal in #1506.