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

Inner, outer class consistency #321

Merged

Conversation

trappitsch
Copy link
Contributor

@trappitsch trappitsch commented Jan 23, 2022

I thought of tackling issue #15, which is also on the todo list for v1.0 (#190). Inner classes seem to have the advantage that the individual channels, etc., are not exposed directly to the user, so I think it makes more sense. On the todo list of instruments to fix are:

  • instruments/abstract_instrument/optical_spectrum_analyzer
  • instruments/abstract_instrument/oscilloscope
  • instruments/abstract_instrument/power_supply
  • newport/agilis
  • newport/newportesp301
  • tektronix/tekdpo4104
  • tektronix/textds224
  • tektronix/tektds5xx
  • srs/srsdg645

As an example, I started with the srs/srsdg645, mostly to see how it goes and discuss above list. Pinging @scasagrande for ideas, comments, etc.

@coveralls
Copy link

coveralls commented Jan 23, 2022

Coverage Status

Coverage increased (+0.0002%) to 99.483% when pulling 460a701 on trappitsch:inner_outer_classes_consistency into 5cdba01 on Galvant:master.

@scasagrande
Copy link
Contributor

Coverage decreased (-3.0e-05%) to 99.482%

Okay I might need to add a minimum failure delta because that is ridiculous 😂

@trappitsch
Copy link
Contributor Author

@scasagrande Is the consistency idea here to move above classes to inner ones and first SRS example ok in your opinion?

@scasagrande
Copy link
Contributor

Yeah I think that's fine. It's what I did for the FunctionGenerator ABC and that was a bit of a pain to do, so sticking with that will be helpful 😂

@trappitsch
Copy link
Contributor Author

sounds good. i'll have a look and start plowing through some classes :)

@trappitsch
Copy link
Contributor Author

The Thorlabs APT seems already adequately specified. Must have missed this in the first go-around, removing it from above list.

@trappitsch
Copy link
Contributor Author

The abstract instrument SignalGenerator uses single and multi channel. I think it might be more appropriate to fix inner-outer class consistency when working on issue #156. Does this make sense @scasagrande?

@trappitsch
Copy link
Contributor Author

trappitsch commented Jan 24, 2022

Looks like coveralls contradicts itself now. Most of the failed coverage seem to come when numpy is None, however, only in the Unit Test task...

@trappitsch trappitsch changed the title [WIP] Inner, outer class consistency Inner, outer class consistency Jan 24, 2022
@trappitsch
Copy link
Contributor Author

The classes I found that had inner-outer consistencies are now changed. I think this is ready for review @scasagrande

@scasagrande
Copy link
Contributor

scasagrande commented Jan 24, 2022 via email

@scasagrande
Copy link
Contributor

Coveralls has been fixed, just refresh your branch 👍

@trappitsch
Copy link
Contributor Author

Yey, even coverall passes now :)

Copy link
Contributor

@scasagrande scasagrande left a comment

Choose a reason for hiding this comment

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

lgtm!

@scasagrande scasagrande merged commit aa51148 into instrumentkit:master Jan 26, 2022
@trappitsch trappitsch deleted the inner_outer_classes_consistency branch January 26, 2022 15:55
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.

3 participants