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

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

Merged
merged 6 commits into from
Mar 17, 2023

Conversation

dougthor42
Copy link
Contributor

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?

timeout, _ = self.get_attribute(constants.ResourceAttribute.timeout_value)
timeout /= 1000

now = start = time.time()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is technically a minor code difference from the other read methods, but the functionality of the code is exactly the same.

@MatthieuDartiailh
Copy link
Member

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

I will try to review ASAP.

@MatthieuDartiailh
Copy link
Member

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
Copy link
Member

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

@MatthieuDartiailh
Copy link
Member

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
Copy link
Contributor Author

Done!

@MatthieuDartiailh MatthieuDartiailh merged commit 2aab58b into pyvisa:main Mar 17, 2023
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