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

Fix mypy error in channels.py #77

Merged
merged 4 commits into from
Mar 13, 2023
Merged

Conversation

dougthor42
Copy link
Contributor

This technically fixes the mypy error seen in channels.py, but I'm not sure if it's the best fix. IMO it simply masks the symptom rather than fixes the root cause. I think more refactoring and architecture changes might be needed to fix the root issue.

$ mypy pyvisa_sim/
pyvisa_sim/channels.py:199: error: Argument 1 to "set_value" of "Property" has incompatible type "Union[Any, Dict[Any, Any]]"; expected "str"  [arg-type]

I also move the type hints to component.Component.__init__ as that's common practice.

One of the issues is that the stringparser library doesn't have type annotations, so mypy sets parser to Any here:

for name, parser, response, error_response in self._setters:

and then parsed to Any here:

parsed = parser(q)

From there, the isinstance statement end up doing type narrowing to Dict[Any, Any] which then gets Union'd with Any from before.

The issue is that the self.set_value function doesn't accept Any. Based on my (possibly flawed) understanding of the code, it looks like it's safe enough to just convert to a string before calling set_value. So that's what I do.

In addition, I ran a temporary backport of this change to v0.5.1 and ran my tests for a separate project that uses pyvisa-sim, and those still ran OK so... I guess it's OK?

@MatthieuDartiailh
Copy link
Member

I personally prefer to avoid crowding type annotations in init so I would appreciate if you could revert your second commit. Additionally, while we are dealing with linter errors could you address the black issues ?

@dougthor42
Copy link
Contributor Author

Done.

Not sure why pre-commit didn't catch the black issue... ah well, a problem for another time.

@MatthieuDartiailh MatthieuDartiailh merged commit c014f01 into pyvisa:main Mar 13, 2023
@dougthor42 dougthor42 deleted the fix-mypy branch March 13, 2023 19:56
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