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

Add small bit of type hinting #1745

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

BryceGattis
Copy link
Contributor

No description provided.

@BryceGattis BryceGattis requested a review from a team as a code owner May 2, 2024 13:21
Copy link

codecov bot commented May 2, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 57.93%. Comparing base (36e0537) to head (91f6a28).
Report is 8 commits behind head on main.

❗ Current head 91f6a28 differs from pull request most recent head 3864fc3. Consider uploading reports for the commit 3864fc3 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1745      +/-   ##
==========================================
- Coverage   58.29%   57.93%   -0.36%     
==========================================
  Files         126      126              
  Lines       17159    17162       +3     
  Branches     3505     3505              
==========================================
- Hits        10002     9942      -60     
- Misses       6493     6550      +57     
- Partials      664      670       +6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: BryceGattis <[email protected]>
Signed-off-by: BryceGattis <[email protected]>
Signed-off-by: BryceGattis <[email protected]>
@@ -235,7 +239,7 @@ def config_schema(self):
deep_update(d, d_)
return dict_to_schema(d, required=True, modifier=expand_system_vars)

def create_instance(self, plugin, **instance_kwargs):
def create_instance(self, plugin, **instance_kwargs) -> "RezPluginType":

Choose a reason for hiding this comment

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

This doesn't return a RezPluginType instance. It will return something like rezplugins.build_system.cmake.CMakeBuildSystem. I don't think we have an easy way today to get all the possible base classes for each plugin type. It should be possible, but it'll require changes to the plugin system, or some more thoughts to maybe use generics or whatever type magic we'll need to get this to work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@JeanChristopheMorinPerso Ah right good point. Would it be alright to type hint these just as object instead for now? That at least gives us some kind of information.

@@ -802,7 +804,7 @@ def _get_new_session_popen_args(self):

class _PluginConfigs(object):
"""Lazy config loading for plugins."""
def __init__(self, plugin_data):
def __init__(self, plugin_data: typing.Dict[str, typing.Any]):
self.__dict__['_data'] = plugin_data
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm curious why we do this self.__dict__['_data'] accessing here. Perhaps there is a cleaner way to do this?

Choose a reason for hiding this comment

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

I think it's to bypass __getattr__.

@chadrik chadrik mentioned this pull request May 21, 2024
BryceGattis and others added 2 commits July 8, 2024 07:38
Co-authored-by: Jean-Christophe Morin <[email protected]>
Signed-off-by: Bryce Gattis <[email protected]>
Co-authored-by: Jean-Christophe Morin <[email protected]>
Signed-off-by: Bryce Gattis <[email protected]>
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