search

pyvisa / pyvisa-sim

A PyVISA backend that simulates a large part of the "Virtual Instrument Software Architecture" (VISA_)
https://pyvisa-sim.readthedocs.io/en/latest/
MIT License
69 stars 39 forks source link

Fix mypy error in channels.py #77

Closed dougthor42 closed 1 year ago

dougthor42 commented 1 year ago

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:

https://github.com/pyvisa/pyvisa-sim/blob/b0c22826ad506cc5c5acc155bdca3764dfe14f5e/pyvisa_sim/channels.py#L187

and then parsed to Any here:

https://github.com/pyvisa/pyvisa-sim/blob/b0c22826ad506cc5c5acc155bdca3764dfe14f5e/pyvisa_sim/channels.py#L189

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 commented 1 year ago

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 commented 1 year ago

Done.

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