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

Consolidate read/write methods in `sessions` modules to the `session.Session` parent class. #75

Closed dougthor42 closed 1 year ago

dougthor42 commented 1 year ago

This is some pre-work for #72. I noticed that there was a fair amount of code duplication in the various sessions modules, so I cleaned that up by moving any pure duplicates to the session.Session parent class.

I also move some common USB code to a BaseUSBSession class.

The only one unchanged was serial.py as those read/write methods are different. I haven't investigated if the need to be different though.

Hopefully test coverage is sufficient to ensure that this doesn't break things. We have line coverage for the write methods, but are missing a couple lines in read.

Comments and Open Questions

  1. I noticed that mypy is failing for channels.py:199. I figured that could be fixed in a separate PR. LMK if it needs to be fixed before merging this.
  2. Do you want me to add anything to `CHANGES.rst?
MatthieuDartiailh commented 1 year ago

If you have the bandwidth to do so I would be very keen on you fixing mypy.

I will try to review ASAP.

MatthieuDartiailh commented 1 year ago

LGTM

The behavior of serial is impacted by a couple of other visa attributes so it likely cannot use the same impl as the others.

MatthieuDartiailh commented 1 year ago

Once you rebase on latest main I will re-check and merge.

MatthieuDartiailh commented 1 year ago

Actually thinking a tiny bit more about this, there is one minor issue.

Technically not all sessions support read and write (register based devices). We do not support them yet (and we may never do so) but to be a tad more future proof I would feel better extracting the logic in a MessageBasedSession that could then be inherited by tcpip, usb, gpib and serial resources. Would you mind making such a change ?

dougthor42 commented 1 year ago

Done!