Driver / signalhound use standard logging#1154
Driver / signalhound use standard logging#1154WilliamHPNielsen merged 9 commits intomicrosoft:masterfrom
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1154 +/- ##
==========================================
+ Coverage 79.78% 79.78% +<.01%
==========================================
Files 48 48
Lines 6657 6663 +6
==========================================
+ Hits 5311 5316 +5
- Misses 1346 1347 +1 |
| """ | ||
| This is a direct port of the signal hound QTLab driver by Ramiro | ||
| Edited by Adriaan Rol | ||
| Edited by Adriaan Rol and others (use git blame for details) |
There was a problem hiding this comment.
what is this comment useful for? i'd remove it since "git blame" is supposedly a known feature of git :)
There was a problem hiding this comment.
Yeah, I was trying to humorously point out that information about who wrote what does not really belong in a driver... From your comment I guess that I failed :) You can remove the whole shebang if you'd like.
There was a problem hiding this comment.
😸 ok, i get it. well, I think that such a note is good fun for github comments, but should not be part of the code. So, I suggest to remove this line.
| This driver is functional but not all features have been implemented. | ||
|
|
||
| TODO: | ||
| Add tracking generator mode |
There was a problem hiding this comment.
can this line be reformulate from "TODO" to smth like : "Note that this driver does not support tracking generator mode". and then perhaps make a separate low-prio issue for "enabling tracking generator mode for ??? driver"? This will make the code distraction-free and potential-ambiguity-free
There was a problem hiding this comment.
No, I think it's better kept as it is. Missing driver features are non-issues until they become issues, if you know what I mean.
There was a problem hiding this comment.
ok, agree. still, when I see "TODO", I understand it as developer-related thing, or a notion of a bug or deficiency. When the statement is formulated as smth like "Note that this driver does not support tracking generator mode", as a user I know that it hasn't been an issue so far and I do not expect the feature to be implemented UNLESS as a user I make an issue out of it on github.
|
|
||
| t1 = time() | ||
| # poor-man's connect_message. We could overwrite get_idn | ||
| # instead and use connect_message. |
There was a problem hiding this comment.
does this supposed to be an issue?
There was a problem hiding this comment.
then why making the comment? :)
| self.log.info('Closing Device with handle num: ', | ||
| self.deviceHandle.value) | ||
| log.info('Closing Device with handle num: ' | ||
| f'self.deviceHandle.value') |
There was a problem hiding this comment.
shouldn't it be f'{self.deviceHandle.value}' (with { and })?
There was a problem hiding this comment.
Yes. Good one.
|
@WilliamHPNielsen Would be good to get this merged |
…Nielsen/Qcodes into fix/driver/signalhound_logging
|
Once CI passes, let's merge. |
Merge: 8274239 880a751 Author: William H.P. Nielsen <[email protected]> Merge pull request #1154 from WilliamHPNielsen/fix/driver/signalhound_logging
3 participants
Fixes #415 and fixes #292
Changes proposed in this pull request:
@QCoDeS/core @AdriaanRol