-
Notifications
You must be signed in to change notification settings - Fork 12
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
Applications: New settings #220
Conversation
Co-authored-by: Roy Nieterau <[email protected]>
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 good to me. I did have two cosmetic notes that would be nice to include.
@BigRoy @iLLiCiTiT Sorry for my mega commit. |
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.
Very nice @MustafaJafar - some remarks.
Also, you seem to space out the question mark with a space at the end of the sentence?
So you do:
- this ?
- instead of this?
Which seems odd. As far as I know that's wrong in English, so would be good to correct.
Co-authored-by: Roy Nieterau <[email protected]>
Co-authored-by: Roy Nieterau <[email protected]>
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've added few commits and resolved previous conversations
But, I'm wondering about the following
Co-authored-by: Roy Nieterau <[email protected]>
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.
As mentioned in nuke supported versions | Ynput Forums
we should emphasize that we don't support apps with python 2.
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 think this PR is ready to merge.
Changelog Description
Added information about new settings introduced in applications addon.
Additional info
Note: The 1.0.0 release is not yet out, should we wait for the release and then merge, or merge and then release?