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.

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 type change in ArtifactIndex (Closes #1681) #1682

Merged
merged 7 commits into from
May 11, 2020

Conversation

umesh-timalsina
Copy link
Contributor

No description provided.

@brollb
Copy link
Contributor

brollb commented May 6, 2020

For clarity, this enables users to modify the type information in an artifact's metadata. It does not change the associated data in any way.

Copy link
Contributor

@brollb brollb left a comment

Choose a reason for hiding this comment

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

The commit message comment is more pressing. The other issue is minor and a comment on legibility/maintainability but it isn't blocking the merge.

@umesh-timalsina
Copy link
Contributor Author

umesh-timalsina commented May 6, 2020

For more clarity on the ramifications of changing the Data Type for an artifact, It might be better to include a ConfirmDialog of some sort as well, which is added in 7a0ec68.

umesh-timalsina and others added 2 commits May 7, 2020 16:25
…change

1. Change meaninfule commit message when changing the attribute of an Artifact Node.
2. Add a ConfirmDialog with warnings for artifact data type Change and change
it only if confirmed.
Copy link
Contributor

@brollb brollb left a comment

Choose a reason for hiding this comment

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

One minor comment. Otherwise, it looks good!

if (newVal && newVal !== oldVal) {
const confirmed = opts.confirmation ? await opts.confirmation.call(this,
opts.nodeName,
{newValue: newVal, oldValue: oldVal}
Copy link
Contributor

Choose a reason for hiding this comment

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

As a minor point, I am not a fan of packing arguments into an options object unless there are a lot of optional arguments. Otherwise, it obscures the method signature without any real benefit.

@umesh-timalsina umesh-timalsina force-pushed the 1681-artifacts-type-change branch from 7e280a9 to f425b7d Compare May 8, 2020 15:00
@brollb brollb merged commit 361720f into master May 11, 2020
@brollb brollb deleted the 1681-artifacts-type-change branch May 11, 2020 14:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants