-
Notifications
You must be signed in to change notification settings - Fork 108
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
Allow user to edit Dataset Name #3207
Allow user to edit Dataset Name #3207
Conversation
dashboard/src/modules/components/EmptyPageComponent/NoMatchingPage.jsx
Outdated
Show resolved
Hide resolved
dashboard/src/modules/components/OverviewComponent/common-component.jsx
Outdated
Show resolved
Hide resolved
9070ba1
to
8adfa69
Compare
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.
Another breakdown in GitHub/Jenkins communication: GitHub knows nothing about it, but Jenkins says it completed successfully over 5 hours ago.
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.
These changes look OK, but I've called out some things for you to consider.
There are a number of spots where Dave and I have suggested using symbolic references rather than raw strings. What I think we really want in these instances are enumerations. A quick Google search suggested several things that you can use to implement enumerations in Javascript; the basic thrust is to define an object or class with keys or attributes which map or evaluate to the raw values, and then refer to those values with, e.g., ROW_SEEN.SEEN
or ROW_SEEN.UNSEEN
instead of "seen-row"
and "unseen-row"
, and, by defining a separate object or class for each usage case, you keep your constant values grouped together but separate from other groups.
I resubmitted the tests, and they passed this time (the previous failure was our favorite flakey Server functional test).
dashboard/src/modules/components/OverviewComponent/common-component.jsx
Outdated
Show resolved
Hide resolved
dashboard/src/modules/components/OverviewComponent/common-component.jsx
Outdated
Show resolved
Hide resolved
8adfa69
to
1fec7f4
Compare
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.
No changes. Varshini and Webb need to work out how to handle his multitude of comments, but it looks OK to me.
Allow user to change dataset.name Modify the name of the dataset using PUT /datasets/metadata API
This reverts commit bb3eb74.
1e33e19
to
4e3772c
Compare
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 like the new function comments. I'd like to see more of these, with more detail, but that's not work for this PR.
I'm OK with this, although I still see at least one case of a string literal value that probably ought to become a compiler-checked constant at some point (newRuns
)... but at this point I'm ready to move on. (Please continue to consider these issues as you work on other changes!)
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.
Let's get this in and then iterate.
Like Dave, I appreciate the function header comments (although, there were some mysterious trailing asterisks in some of the lines), and there is at least one string ("error"
) which should have been converted to a constant. In addition, there are a few places among the code changes where you're defining an arrow functions which are yielding code blocks that immediately return
instead of just yielding the corresponding expression instead. But, these are small things which can be attended to over time in subsequent changes.
PBENCH-1017
Allow user to change dataset.name
The server permits the owner of a dataset to modify the name of the dataset using PUT /datasets/metadata