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

full typing with mypy #16

Merged
merged 13 commits into from
Sep 9, 2024
Merged

full typing with mypy #16

merged 13 commits into from
Sep 9, 2024

Conversation

sigma67
Copy link
Contributor

@sigma67 sigma67 commented Aug 8, 2024

currently work in progress, let me know if you are interested in continued work on this @madebr

About 19 findings are outstanding, maybe some more if we turn on strict mode

@madebr
Copy link
Owner

madebr commented Aug 8, 2024

This is good work to increase confidence in correctness!
I recently ran a few scripts through mypy and it caught real bugs without having to go through testing.

@sigma67
Copy link
Contributor Author

sigma67 commented Aug 8, 2024

Ok great then I can continue.

What do you think should be the minimum Python version in 2024? (looks like 3.8 from the tests)

I need to make sure the typing syntax is compatible with the minimum supported Python version.

@madebr
Copy link
Owner

madebr commented Aug 8, 2024

I like to support all python versions, also supported by cpython, which is currently >=3.8.
It looks like 3.8 ends October this year, so I'd be fine with >=3.9.
https://www.python.org/downloads/

__slots__ = ("name", "value")

def __init__(self, name: str, value: str):
self.name = name
self.value = value

@classmethod
def from_dict(cls, dikt: Dict) -> "CacheEntryProperty":
def from_dict(cls, dikt: dict) -> "CacheEntryProperty":
Copy link
Owner

Choose a reason for hiding this comment

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

I think it's now possible to remove the unused typing imports?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

self.name = name
self.value = value
self.type = type
self.properties = properties

@classmethod
def from_dict(cls, dikt: Dict) -> "CacheEntry":
def from_dict(cls, dikt: dict) -> "CacheEntry":
Copy link
Owner

Choose a reason for hiding this comment

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

Also, does the dikt need a more specialized typing annotation?
e.g. dict[str, object]?

I see you've changed it into dict[str, Any] in other locations.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, needs to be changed everywhere to comply. Long-term it would be great to replace the Any as well, perhaps

Copy link
Owner

Choose a reason for hiding this comment

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

When/if that becomes possible, you'd effectively replace a lot of work these classes do.

@sigma67
Copy link
Contributor Author

sigma67 commented Aug 8, 2024

Here are the last remaining issues with mypy strict mode, they are a bit tougher to resolve. Any suggestions?

~/PycharmProjects/python-cmake-file-api py-typed !9 ?1 ❯ mypy --install-types --non-interactive --strict                                                                                                                                                                       🐍 python-cmake-file-api-3.10 17:30:38
cmake_file_api/kinds/configureLog/target/v2.py:496: error: Argument 4 to "CodemodelTargetV2" has incompatible type "BacktraceNode | None"; expected "BacktraceNode"  [arg-type]
cmake_file_api/kinds/codemodel/target/v2.py:297: error: Argument 2 to "TargetCompileGroupPCH" has incompatible type "Any | None"; expected "BacktraceNode"  [arg-type]
cmake_file_api/kinds/codemodel/target/v2.py:320: error: Argument 2 to "TargetCompileGroupDefine" has incompatible type "Any | None"; expected "BacktraceNode"  [arg-type]
cmake_file_api/kinds/codemodel/target/v2.py:497: error: Argument 4 to "CodemodelTargetV2" has incompatible type "Any | None"; expected "BacktraceNode"  [arg-type]
cmake_file_api/reply/v1/api.py:54: error: "object" has no attribute "keys"  [attr-defined]
cmake_file_api/reply/v1/api.py:78: error: "object" has no attribute "get"  [attr-defined]
cmake_file_api/reply/v1/api.py:81: error: Returning Any from function declared to return "CMakeFilesV1 | None"  [no-any-return]
cmake_file_api/reply/v1/api.py:87: error: Need type annotation for "result" (hint: "result: dict[<type>, <type>] = ...")  [var-annotated]
cmake_file_api/reply/v1/api.py:89: error: "object" has no attribute "get"  [attr-defined]
Found 9 errors in 3 files (checked 38 source files)

@sigma67 sigma67 changed the title wip: full typing with mypy full typing with mypy Aug 8, 2024
@madebr
Copy link
Owner

madebr commented Aug 8, 2024

Here are the last remaining issues with mypy strict mode, they are a bit tougher to resolve. Any suggestions?

I will look into these asap.
Thanks for your work :)

@sigma67
Copy link
Contributor Author

sigma67 commented Aug 26, 2024

Hi @madebr , could you give some feedback on the findings? I can fix them myself if needed

@madebr
Copy link
Owner

madebr commented Sep 9, 2024

I see one remaining typing bug, but don't know how to address the others.
It looks like mypy needs additional context.

--- a/cmake_file_api/kinds/configureLog/target/v2.py
+++ b/cmake_file_api/kinds/configureLog/target/v2.py
@@ -434,7 +434,7 @@ class CodemodelTargetV2:
                  "isGeneratorProvided", "install", "link", "archive", "dependencies", "sources",
                  "sourceGroups", "compileGroups")
 
-    def __init__(self, name: str, id: str, type: TargetType, backtrace: BacktraceNode, folder: Optional[Path],
+    def __init__(self, name: str, id: str, type: TargetType, backtrace: Optional[BacktraceNode], folder: Optional[Path],
                  paths: CMakeSourceBuildPaths, nameOnDisk: str, artifacts: list[Path],
                  isGeneratorProvided: Optional[bool], install: Optional[TargetInstall],
                  link: Optional[TargetLink], archive: Optional[TargetArchive],

@sigma67
Copy link
Contributor Author

sigma67 commented Sep 9, 2024

@madebr everything fixed up and ready to go :)

@madebr madebr merged commit 067fd06 into madebr:master Sep 9, 2024
13 checks passed
@madebr
Copy link
Owner

madebr commented Sep 9, 2024

Thanks for the non-trivial fix :)

I just pushed v0.0.8, which will be available on pypi soon

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