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.

Sign up for GitHub

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

Jump to bottom

Upgrade AMI drivers to conform to our standard #4536

Merged
merged 6 commits into from
Aug 30, 2022
Merged

Upgrade AMI drivers to conform to our standard #4536

merged 6 commits into from
Aug 30, 2022

Conversation

jenshnielsen
Copy link
Collaborator

No description provided.

@jenshnielsen jenshnielsen added this to the Public api rewrite milestone Aug 21, 2022
@jenshnielsen jenshnielsen added driver docs Related to docs improvements labels Aug 21, 2022
@jenshnielsen jenshnielsen mentioned this pull request Aug 21, 2022
Closed
@codecov
Copy link

codecov bot commented Aug 21, 2022 •
edited
Loading

Codecov Report

Merging #4536 (f223109) into master (f25e4a6) will increase coverage by 0.02%.
The diff coverage is 92.85%.

@@            Coverage Diff             @@
##           master    #4536      +/-   ##
==========================================
+ Coverage   68.05%   68.08%   +0.02%     
==========================================
  Files         299      299              
  Lines       31511    31535      +24     
==========================================
+ Hits        21446    21470      +24     
  Misses      10065    10065

@jenshnielsen
Copy link
Collaborator Author

The issue seems to be caused by the AMI430.py file next to the visa fill. Need to debug why the stops sphinx looking for classes in the mro

@jenshnielsen
Copy link
Collaborator Author

jenshnielsen commented Aug 26, 2022 •
edited
Loading

Thinking a bit more about this. Its not strange that this trips up. Both the old Module and the class are called AMI430 so sphinx ends up trying to import the old AMI430 module rather than the class. This also has a AMI430 class but which inherits from ipinstrument so its lacking the expected attributes.

Not sure what the best is.

  1. Rename the old module to AMI_430.py perhaps. This would be the cleanest but is also a breaking change.
  2. Rename the class as exported to something else. Can be done without breaking changes but is less ideal

Edit Decided to change the name of the public class at the top level to avoid this and reflect that the instrument is called AMI Model 430

@astafan8 would be great if you can have a look

@jenshnielsen
Copy link
Collaborator Author

bors merge

@bors
Copy link
Contributor

bors bot commented Aug 30, 2022

Build succeeded:

@bors bors bot merged commit 962a0d9 into microsoft:master Aug 30, 2022
@jenshnielsen jenshnielsen deleted the driver/ami branch August 30, 2022 12:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@astafan8 astafan8 astafan8 approved these changes

Assignees
No one assigned
Labels
docs Related to docs improvements driver
Projects
None yet
Milestone
Public api rewrite
Development

Successfully merging this pull request may close these issues.
2 participants
@jenshnielsen @astafan8