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

Update "Use task handles" instance attribute label #94

Merged
merged 1 commit into from
Sep 10, 2024

Conversation

BigRoy
Copy link
Contributor

@BigRoy BigRoy commented Sep 10, 2024

Changelog Description

Update instance attribute label "Use Asset Handles" to "Use Task Handles"

Additional info

This makes it match in wording with the validation itself AND the repair action's label that reads "Disable use Task Handles"

It's basically cosmetics.

Testing notes:

  1. Publish an instance
  2. Toggle on the instance should make sense.

@MustafaJafar MustafaJafar added the type: maintenance Changes to the code that don't affect product functionality (Technical debt, refactors etc.)) label Sep 10, 2024
Copy link
Contributor

@MustafaJafar MustafaJafar left a comment

Choose a reason for hiding this comment

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

I wonder if should we refactor the plugin name. and fix it in settings too.

Will that be compatible with existent instances ?
Edit: surprisingly, yes as it won't affect any current instances because we use use_handles publish attribute.
publish attributes:

    "CollectAssetHandles": {
        "use_handles": true
    },

I've refactored names from asset to task completely.
find it in this branch enhancement/collect_task_handles_label_refactor_further_more
feel free to fetch and merge it if you find it useful.

Also, I tested changes in my branch it seems to work as usual.
image
image

Copy link
Contributor

@MustafaJafar MustafaJafar left a comment

Choose a reason for hiding this comment

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

This PR works fine since we don't refactor the plugin class name therefore the settings works as usual.

But, if you would like to test my suggested changes, please find it in my previous comment.
which also was tested and works fine on my side. so feel free to merge the PR at its current state or after merging my suggestions.

@BigRoy
Copy link
Contributor Author

BigRoy commented Sep 10, 2024

Edit: surprisingly, yes as it won't affect any current instances because we use use_handles publish attribute.
publish attributes:

Actually - that branch shouldn't be backwards compatible since it's stored in publish_attributes["CollectAssetHandles"] before but publish_attributes["CollectTaskHandles"] after. I feel like it's not worth it to lose backwards compatibility over. I'll merge this PR since yours continues on it anyway and we can debate more if we want to break that backwards compatibility for the small cleanup on the class name and the settings name.

@BigRoy BigRoy merged commit 0dc54e6 into ynput:develop Sep 10, 2024
1 check passed
@BigRoy BigRoy deleted the enhancement/collect_task_handles_label branch September 10, 2024 17:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: maintenance Changes to the code that don't affect product functionality (Technical debt, refactors etc.))
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants