-
Notifications
You must be signed in to change notification settings - Fork 55
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
Added ArrayImage to pyface.api as per #972 #997
Conversation
Updated deprecated font-related aliases on line 34
@corranwebster @rahulporuri checkout this PR. |
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.
This PR is almost ready. Apart from the comment below, can you also add ArrayImage
to the module docstring at the top of the file? ArrayImage
can be included in the "Miscellaneous" section.
pyface/api.py
Outdated
@@ -112,7 +112,7 @@ | |||
from .layered_panel import LayeredPanel | |||
from .message_dialog import error, information, warning, MessageDialog | |||
from .progress_dialog import ProgressDialog | |||
|
|||
from .array_image import ArrayImage |
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.
The imports are sorted alphabetically in this file so can you please update the location of this import statement?
Added `ArrayImage` to the module docstring and also updated the location of it's import statement.
@rahulporuri It's fine now right?? |
There's a technical gotcha in this which I think @rahulporuri is the person to make a decision. Currently I think that NumPy is technically an optional dependency of Pyface with the Qt backend (it is required for Wx for various historical reasons). This change means the NumPy is required when importing Lines 118 to 123 in 997c78f
but with numpy in the place of Pygments. Alternatively @rahulporuri may say that depending on Lines 21 to 27 in 997c78f
This is why I didn't put |
@corranwebster @rahulporuri Soo what to do with this PR now? Do I need to do more modifications in it, or will it be merged after some time? |
@Palash-Vishnani sorry for the delayed response but @corranwebster is right. I missed it when i first reviewed this PR. Can you make the changes that corran recommended i.e. something like this
Can you please add this new import next to the other optional imports? |
@rahulporuri I'll do it but should I remove this import satement "Note that the :class: |
Yes. You need to replace the
Yes please. That's right. You need to add this information in the module docstring to make it clear to end users. |
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.
LGTM. Merging now.
Thanks @Palash-Vishnani for the contribution! If you're interested in doing more, you can also add the |
No description provided.