-
Notifications
You must be signed in to change notification settings - Fork 7
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
LIU-416: Setup human readable keys for graph UI #293
Conversation
This comment was marked as off-topic.
This comment was marked as off-topic.
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 @myxie - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 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.
daliuge-common/dlg/utils.py
Outdated
if len(second_el) > readableLengthLimit: | ||
split[1] = split[1][0:readableLengthLimit] | ||
truncatedUid = split[1] | ||
truncatedUid = split[1] |
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.
issue (bug_risk): Logic error in truncateUidToKey - modified value is never used
The truncated value is assigned but then immediately overwritten by the original split[1]. This makes the length check ineffective.
daliuge-common/dlg/utils.py
Outdated
readableLengthLimit = 4 | ||
split = uid.split("_") | ||
if len(split) > 1: | ||
second_el = str(split[1]) | ||
if len(second_el) > readableLengthLimit: | ||
split[1] = split[1][0:readableLengthLimit] |
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.
issue (code-quality): We've found these issues:
- Move assignments closer to their usage (
move-assign
) - Replace a[0:x] with a[:x] and a[x:len(a)] with a[x:] (
remove-redundant-slice-index
)
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.
For now this is far better than the UIDs obviously. Once we work more seriously on the translator we should consider to get back closer to the original naming scheme, where the drops had been counted separately inside constructs, leading to names like *_n_m, where n was the running number of the construct drop and m the running number inside the construct.
I have added the construct metadata to the keys now, too, so we get the |
JIRA Ticket
Type
Problem/Issue
By moving to UUIDs for our Drops OIDs, we have made the UI a lot more cluttered:
This makes it a) harder to read what node is what on the graph, and b) hard to relate a FileDrop node on the graph with the output file created by it:
Solution
Checklist
[ ] Unittests added[ ] Documentation addedSummary by Sourcery
Implement human-readable keys for graph UI elements to enhance visualization and file navigation, and introduce a utility function for generating truncated UIDs.
New Features:
Enhancements:
Summary by Sourcery
Implement human-readable keys for graph UI elements to enhance visualization and file navigation, and introduce a utility function for generating truncated UIDs.
New Features:
Enhancements: