-
-
Notifications
You must be signed in to change notification settings - Fork 10
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
fix(config): update mypy configuration #117
Conversation
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
There is linter error in this line, related to subscriptable.
(suggestion from Claude) |
https://github.com/osl-incubator/makim/actions/runs/11099645303/job/30834203281?pr=117#step:6:17 @abhijeetSaroha for |
Ok...I will do that and will update you. |
@abhijeetSaroha could you rebase this branch on top of upstream main please? |
f8c478c
to
f5fbd8f
Compare
I've successfully rebased the branch on top of the upstream main. Regarding the other PR, thank you for merging it! The tests for the post-run are already in place. https://github.com/osl-incubator/makim/blob/main/tests/smoke/.makim-hooks.yaml#L23C1-L36C32 Is it OK ? |
for some reason I missed that. that is ok. appreciate that! |
src/makim/console.py
Outdated
|
||
def get_terminal_size( | ||
default_size: Tuple[int, int] = (80, 24), | ||
) -> Tuple[int, int]: |
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 could be tuple
if you import from __future__ import annotations
but I will not block this PR to be merged because of this
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.
actually there are two more changes for this PR before it is ready to go .. so please could you already fix this one as well?
src/makim/core.py
Outdated
|
||
return env, variables | ||
|
||
def _load_scoped_vars(self, scope: str, env) -> dict: | ||
def _load_scoped_vars(self, scope: str) -> Any: |
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 would say that this should be a dict[str, Any]
def _load_scoped_vars(self, scope: str) -> Any: | |
def _load_scoped_vars(self, scope: str) -> dict[str, Any]: |
and in order to fix the issue with the return
I think it should be something like this: return cast(Dict[str, Any], fix_dict_keys_recursively(variables))
it is still not perfect .. but I will think some ways to improve fix_dict_keys_recursively
.. but for now please do this change.
src/makim/core.py
Outdated
@@ -434,19 +437,22 @@ def _load_scoped_vars(self, scope: str, env) -> dict: | |||
|
|||
return fix_dict_keys_recursively(variables) |
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.
return fix_dict_keys_recursively(variables) | |
return cast(Dict[str, Any], fix_dict_keys_recursively(variables)) |
src/makim/console.py
Outdated
|
||
def get_terminal_size( | ||
default_size: Tuple[int, int] = (80, 24), | ||
) -> Tuple[int, int]: |
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.
actually there are two more changes for this PR before it is ready to go .. so please could you already fix this one as well?
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! thanks for working on that, @abhijeetSaroha
🎉 This PR is included in version 1.17.0 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
Solves #116