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

API: Add DelatedKeyboardInterrupt and depreacate to api #4309

Merged
merged 7 commits into from
Jun 24, 2022

Conversation

jenshnielsen
Copy link
Collaborator

A few questions

  • The deprecate module contains 2 methods to assert deprecation messages. One of those were used in test_station. I have replaced that with the warnings testing logic from pytest. I think that using the pytest code is cleaner and more portable so I suggest that we only use that going forward and don't document these methods.
  • test_station still makes use of the function to generate a test message deprecation_message I don't however think this should be part of the public api. We could rewrite the tests to not make use of this function?

@codecov
Copy link

codecov bot commented Jun 24, 2022

Codecov Report

Merging #4309 (ff80184) into master (982828e) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #4309      +/-   ##
==========================================
- Coverage   68.44%   68.43%   -0.01%     
==========================================
  Files         254      254              
  Lines       30969    30967       -2     
==========================================
- Hits        21196    21192       -4     
- Misses       9773     9775       +2     

Copy link
Contributor

@astafan8 astafan8 left a comment

Choose a reason for hiding this comment

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

The deprecate module contains 2 methods to assert deprecation messages. One of those were used in test_station. I have replaced that with the warnings testing logic from pytest. I think that using the pytest code is cleaner and more portable so I suggest that we only use that going forward and don't document these methods.

agree with using pytest's syntax and don't expose those assertion methods as public api.

test_station still makes use of the function to generate a test message deprecation_message I don't however think this should be part of the public api. We could rewrite the tests to not make use of this function?

deprecation_message indeed should not be part of the qcodes api for use outside of qcodes. our users should only want to silence/catch/etc the specific qcodes warning, and maybe use the convenient issue warning and decorate functions (but that I could also doubt about). However, i don't see any harm in using this function INSIDE qcodes, even in the tests (where actually matching on the "message" and "alternative" parts of the warning would be enough).

qcodes/tests/test_station.py Outdated Show resolved Hide resolved
Co-authored-by: Mikhail Astafev <astafan8@gmail.com>
@jenshnielsen
Copy link
Collaborator Author

bors merge

@bors bors bot merged commit 049c0e4 into microsoft:master Jun 24, 2022
@jenshnielsen jenshnielsen deleted the api/keyboardinterrupt branch June 24, 2022 09:47
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