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

Tweaks to NoteInputDialog #47

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

manneohrstrom
Copy link
Contributor

Improved the docs for the NoteInputDialog.
Changed the constructor to not be open but rather specific in order to make it easier to handle future changes to the interface.

"""
def __init__(self, *args, **kwargs):
def __init__(self, parent):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is how we do it everywhere else in the code. I think in general, we should try to avoid the *args, **kwargs open syntax unless we have reason to use it. It can make it trickier to make changes in the future - say for example we wanted to add some parameters to this method in the future.

"""
Constructor.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

minor: this was looking odd in the sphinx docs!

@manneohrstrom
Copy link
Contributor Author

We have something similar here - do you think it would be worth trying to consolidate the two?

https://github.com/shotgunsoftware/tk-framework-qtwidgets/blob/master/python/activity_stream/dialog_reply.py

Copy link
Contributor

@thebeeland thebeeland left a comment

Choose a reason for hiding this comment

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

Looks good.

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