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 support for dialogs without response #78

Closed dougthor42 closed 1 year ago

dougthor42 commented 1 year ago

Fixes #76.

Re-add support for missing "response" key in the dialogues part and add tests for _get_pair and _get_triplet so that a similar regression won't happen in the future.

Note: Fully fixing the type annotations ended up being a much larger endeavor than expected, so I left that out of this PR and simply used type: ignore.

dougthor42 commented 1 year ago

TL;DR: You can see what all the type changes are in this branch: https://github.com/dougthor42/pyvisa-sim/tree/gh-76-fix-dialog-wo-response-types

Changing the return type of _get_pair from str to OptionalStr ends up cascading down to:

After doing all of that, you get to:

$ mypy pyvisa_sim/
pyvisa_sim/devices.py:258: error: Argument 1 to "extend" of "bytearray" has incompatible type "Union[bytes, Literal[Responses.NO]]"; expected "Iterable[SupportsIndex]"  [arg-type]

https://github.com/pyvisa/pyvisa-sim/blob/6fb7d9d01db76cc647e28f6aaddfcbe1c3b73e1f/pyvisa_sim/devices.py#L256-L258

Which requires it's own type guard if eom is not NoResponse:

                 if response is not NoResponse:
                     self._output_buffer.extend(response)
-                    self._output_buffer.extend(eom)
+                    if eom is not NoResponse:
+                       self._output_buffer.extend(eom)

I'm not yet familiar enough with the code to know if the type guard should be on both self._output_buffer (if response is not NoResponse and eom is not NoResponse) or just on one (like in the diff above). More testing and investigation is required.

In the end I didn't feel it was (a) worth it right now and (b) suitable to fix in this PR.

codecov-commenter commented 1 year ago

Codecov Report

Merging #78 (a15b834) into main (6fb7d9d) will increase coverage by 0.28%. The diff coverage is 100.00%.

:mega: This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##             main      #78      +/-   ##
==========================================
+ Coverage   82.52%   82.81%   +0.28%     
==========================================
  Files          15       16       +1     
  Lines        1076     1094      +18     
  Branches      167      171       +4     
==========================================
+ Hits          888      906      +18     
  Misses        142      142              
  Partials       46       46              
Flag Coverage Δ
unittests 82.81% <100.00%> (+0.28%) :arrow_up:

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
pyvisa_sim/parser.py 73.91% <100.00%> (ø)
pyvisa_sim/testsuite/test_parser.py 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

MatthieuDartiailh commented 1 year ago

Thanks. Type hints are a recent addition and I did not get them all right. I will merge as is but feel free to make a PR regarding teh type annotations and I will have a look.