Initial attempt to create base GUI classes #113
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #113 +/- ##
==========================================
- Coverage 93.48% 93.41% -0.07%
==========================================
Files 10 11 +1
Lines 1856 1943 +87
==========================================
+ Hits 1735 1815 +80
- Misses 121 128 +7 ☔ View full report in Codecov by Sentry. |
| PYPI = 'PyPI' | ||
|
|
||
|
|
||
| class BasePackageMetadata(Protocol): |
There was a problem hiding this comment.
Is this just for the types? Should it be guarded behind TYPE_CHECKING? Not saying that's the best practice, I honestly don't know, but it was weird to see the same schema in this protocol class and also in the named tuple below.
There was a problem hiding this comment.
Checking, I think this is needed as it is so it can be used for the Qt Slot decorator (putting this definition under a TYPE_CHECKING guard raises an error - NameError: name 'BasePackageMetadata' is not defined when using Slot). However, if there is a better way to handle this let me know
There was a problem hiding this comment.
If that's the case let's add a comment about in the docstring, thanks! I'm not aware of any other alternatives.
| self.install_info_button.addWidget(self.info_choice_wdg) | ||
| self.info_choice_wdg.show() | ||
|
|
||
| def _handle_plugin_api_version(self, plugin_api_version): |
There was a problem hiding this comment.
As a rule of thumb, if the base class method is raising NotImplementedError, I think we should explain what's the expected behavior of this method with a docstring.
There was a problem hiding this comment.
Docstring still missing. What should a subclass implement here?
There was a problem hiding this comment.
This is kind of like a hook to customize things for a plugin item just before it finishes being setup (the call for this method is one of the last things done before exiting the class constructor). On the napari implementation, this checks the plugin API and depending on that sets the icon and text that goes besides the plugin name
| CONDA = 'Conda' | ||
| PYPI = 'PyPI' |
There was a problem hiding this comment.
These two globals are also available in the base module. Shall we delete here?
|
I like it a lot @dalthviz. Just added some comments about documenting the pieces that are meant to be subclassed. The base class docstring should detail how to get to a functional plugin manager in the subclass; e.g. "make sure to subclass all the methods that raise |
Co-authored-by: jaimergp <[email protected]>
|
I think this is ready for a new review @jaimergp Let me know what you think! |
| """License information of the package being represented.""" | ||
|
|
||
|
|
||
| class _BasePackageMetadata(NamedTuple): |
There was a problem hiding this comment.
Add docstring here about intent of this NamedTuple, why it is private and how it relates to the protocol above. I wonder: can we use a dataclass for both things and have a single object?
There was a problem hiding this comment.
The main reason why I used the Protocol class + Actual class implementation is that when I tried to subclass the basic class implementation while also subclassing npe2.PackageMetadata errors appeared. Will check if using dataclass instead of inheriting from NamedTuple could help with this.
In the meantime, I think a better selection of names can be done. So something like PackageMetadataProtocol and BasePackageMetadata as well as some info over the doctring as you mention 👍
Edit: Tried using dataclass for the base class but was unable to make it work (subclassing the base class along side npe2.PackageMetadata) :/
| """ | ||
| Movie to use to indicate something is loading. | ||
|
|
||
| This should return an instance of `QMovie` with a scaled size fo 18x18 |
There was a problem hiding this comment.
Can we add the type to the signature?
| ---------- | ||
| info : str | ||
| Info message to be shown. | ||
|
|
There was a problem hiding this comment.
Are these extra blank lines by choice, or enforced by the formatter? If not enforced, I think we should remove them.
| """ | ||
| raise NotImplementedError | ||
|
|
||
| def _trans(self, text, **kwargs): |
There was a problem hiding this comment.
Yep, or at least a subclass of str so I guess this could be set to describe that it returns something like TypeVar('T', bound=str)? I think that should correspond with what napari trans._ returns (Union[TranslationString, str]), right? 🤔
Edit: Actually, I think using AnyStr should be enough? For the moment I leave str but let me know!
There was a problem hiding this comment.
Another round of comments. Let's focus on making sure that the user has the necessary information to subclass the base class. Meaning that all methods meant to be subclassed have a docstring and a properly type-annotated function signature, included return types, if necessary. I think we'll be ready to merge after that <3. Thanks!
Co-authored-by: jaimergp <[email protected]>
| """ | ||
| Protocol class defining the minimum atributtes/properties needed for package metadata. | ||
|
|
||
| This class is meant for type checking propuses as well as to provide a type to use with |
Co-authored-by: Gonzalo Pena-Castellanos <[email protected]>
|
Gonzalo and I were talking that we should probably wait for #51 to land first, and then merge this one, WDYT? |
That makes sense 👍 |
Description
Create a set of base GUI classes so the logic implement can be reused while also keeping an implementation for napari
Notes
Although more work needs to be done here (somehow improve coverage/add test to the base classes for example). Marking as ready to review to get some feedback on the approach used here. Let me know what you think @jaimergp !