-
Notifications
You must be signed in to change notification settings - Fork 9
add @target attribute to NXobject using new field super-type
#378
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
base: fairmat
Are you sure you want to change the base?
Conversation
Reviewer's Guide by SourceryThis pull request introduces a No diagrams generated as the changes look simple and do not need a visual representation. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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.
Hey @lukaspie - I've reviewed your changes - here's some feedback:
Overall Comments:
- The changes to
NXspindispersionshould be in a separate pull request. - Consider renaming the attribute to
NX_TARGETto follow the NeXus naming convention for attributes.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
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.
Looks reasonable to me provided the clarification in the docstring gets added.
Nice to see sourcery being tested in this example for this example here it is too chatty, your docstring to the pr is cleaner and more succinct
base_classes/NXobject.nxdl.xml
Outdated
| </field> | ||
| <attribute name="target" type="NX_CHAR"> | ||
| <doc> | ||
| This attribute can be used to describe that this group is connected to another group. |
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 see, now the nesting,
nevertheless for the docstring
"to another {field, group}" add "- its target -"
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.
Not sure I understand your point, can you please write out what you want to have?
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.
the concept is called @target and FIELDNAME/@target respectively.
It would be nice to pick this up in the docstring and explain like
concept A (the FIELDNAME or GROUP) has a target that is the pointed to other field or group,
i.e. the target the instance data that are written into the concept attribute @target one points at.
Cuz at some points I was wondering why it is called target and not source.
The idea was that the docstring should clarify the direction of the pointing from one concept to another, therefore I wrote "- its (i.e. concept A's) target - "
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.
Ok, makes sense. This is a bit tricky though because for links in HDF5, these are the same object, so for one of them the target attribute will point onto the field/group itself. I added your suggestions, but this language is probably anyway something that NIAC will have to define.
bff858a to
397021e
Compare
The summary at the top also gets created by sourcery, so maybe it's not all bad. |
5bf91a0 to
e3301bd
Compare
Summary by Sourcery
Add a new
@targetattribute to the NXobject base class to support describing connections between fields and groupsNew Features:
@targetattribute that can be used to describe connections between fields and groups, enabling explicit link targetingEnhancements: