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

Use typeddicts for better kwargs typing in Instrument classes #6012

Merged
merged 23 commits into from May 17, 2024

Conversation

jenshnielsen
Copy link
Collaborator

@jenshnielsen jenshnielsen commented Apr 27, 2024

This means that the **kwargs of a Instrument class will exactly match the super class while transparently keeping the ability to add another kwarg to the instrument class when required.

The one downside of this is that it is not possible to override the default value of a kwarg in a subclass since that results in an error about overlapping kwargs. We use this pattern fairly frequently in visa instruments to change the default value of the terminator. In this code we therefor do not add the terminator kwarg to the TypedDict

  • Update docs on writing drivers to reflect this
  • Update docs to reflect use of attribute for terminator and timeout
  • Add docstrings to typed dict classes
  • Migrate drivers until and including instruments vendors starting with K
  • Changelog. Document that all these arguments are now kw only
  • Verify that the change to Instrument is acceptable. (label and metadata is kw only)

In a separate pr

  • Migrate remaining visa drivers in qcodes to the new format

Copy link

codecov bot commented Apr 27, 2024

Codecov Report

Attention: Patch coverage is 92.63566% with 19 lines in your changes are missing coverage. Please review.

Project coverage is 67.61%. Comparing base (d91be17) to head (90451da).

Files Patch % Lines
...des/instrument_drivers/Keysight/Keysight_B2962A.py 71.42% 2 Missing ⚠️
src/qcodes/instrument_drivers/Galil/dmc_41x3.py 66.66% 1 Missing ⚠️
src/qcodes/instrument_drivers/HP/HP_8133A.py 80.00% 1 Missing ⚠️
src/qcodes/instrument_drivers/HP/HP_8753D.py 80.00% 1 Missing ⚠️
...codes/instrument_drivers/Keithley/Keithley_2000.py 80.00% 1 Missing ⚠️
...codes/instrument_drivers/Keithley/Keithley_2400.py 80.00% 1 Missing ⚠️
...codes/instrument_drivers/Keithley/Keithley_6500.py 75.00% 1 Missing ⚠️
...rc/qcodes/instrument_drivers/Keysight/Infiniium.py 80.00% 1 Missing ⚠️
src/qcodes/instrument_drivers/Keysight/KtMAwg.py 80.00% 1 Missing ⚠️
src/qcodes/instrument_drivers/Keysight/N51x1.py 80.00% 1 Missing ⚠️
... and 8 more
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6012      +/-   ##
==========================================
+ Coverage   67.55%   67.61%   +0.06%     
==========================================
  Files         351      351              
  Lines       30490    30549      +59     
==========================================
+ Hits        20597    20656      +59     
  Misses       9893     9893              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jenshnielsen jenshnielsen force-pushed the typed_kwargs branch 3 times, most recently from 3ab2245 to 1972632 Compare April 30, 2024 08:03
@jenshnielsen jenshnielsen changed the title [WIP] Use typeddicts for better kwargs typing in Instrument classes Use typeddicts for better kwargs typing in Instrument classes May 1, 2024
@jenshnielsen jenshnielsen marked this pull request as ready for review May 1, 2024 14:41
@jenshnielsen jenshnielsen requested a review from a team as a code owner May 1, 2024 14:41
@jenshnielsen jenshnielsen force-pushed the typed_kwargs branch 3 times, most recently from 8d37784 to b5d6476 Compare May 13, 2024 08:12
src/qcodes/instrument/instrument.py Show resolved Hide resolved
src/qcodes/instrument/visa.py Show resolved Hide resolved
@jenshnielsen jenshnielsen added this pull request to the merge queue May 17, 2024
Merged via the queue into microsoft:main with commit 239e936 May 17, 2024
20 checks passed
@jenshnielsen jenshnielsen deleted the typed_kwargs branch May 17, 2024 13:19
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.

None yet

2 participants