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

Ticket1826 keithley 2400 #422

Merged
merged 23 commits into from
Jan 31, 2017
Merged

Ticket1826 keithley 2400 #422

merged 23 commits into from
Jan 31, 2017

Conversation

AdrianPotter
Copy link
Contributor

@AdrianPotter AdrianPotter commented Jan 23, 2017 •
edited by John-Holt-Tessella
Loading

Description of work

Add OPI support for the Keithley 2400 source meter

To test

ISISComputingGroup/IBEX#1826

Acceptance criteria

  • OPI for Keithley 2400 available
  • Talks correctly with IOC
  • OPI functions work as expected

Code Review

  • Is the code of an acceptable quality?
  • Does the code conform to the coding standards? Is it well structured with small focussed classes/methods/functions?
  • Are there unit tests in place? Are the unit tests small and test the a class in isolation?
  • Are there system tests in place? Do they test a minimal set of functionality and leave the gui as close as possible to its original state?
  • Did any existing system test break as a result of the current changes?
  • Have the changes been documented in the release notes. If so, do they describe the changes appropriately?
  • Has the manual system tests spreadsheet been updated?
  • If an OPI has been modified, does it conform to the style guidelines? There is a script called check_opi_format.py to help with this.

Functional Tests

  • Do changes function as described? Add comments below that describe the tests performed.
  • How do the changes handle unexpected situations, e.g. bad input?
  • Has developer documentation been updated if required?

Final Steps

  • Reviewer has moved the release notes entry for this ticket in the "Changes merged into master" section

AdrianPotter added 22 commits January 10, 2017 11:07
Tom-Willemsen
Tom-Willemsen reviewed Jan 30, 2017
View reviewed changes
Copy link
Contributor

@Tom-Willemsen Tom-Willemsen left a comment

Choose a reason for hiding this comment

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

OPI looks good - just one tooltip that erroneously refers to the Eurotherm (see comment in code).

Also ensure release notes are added.

base/uk.ac.stfc.isis.ibex.opis/resources/Keithley2400/Keithley2400.opi
</widget>
<widget typeId="org.csstudio.opibuilder.widgets.tab" version="1.0">
<active_tab>0</active_tab>
<tooltip>Disabled tabs are because Eurotherm is not active in IOC.</tooltip>
Copy link
Contributor

Choose a reason for hiding this comment

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

This should read Disabled tabs are because the Keithley 2400 is not active in IOC. rather than Disabled tabs are because Eurotherm is not active in IOC.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed. Clumsy copy and paste error.

@John-Holt-Tessella John-Holt-Tessella merged commit f6a00c6 into master Jan 31, 2017
@John-Holt-Tessella John-Holt-Tessella deleted the Ticket1826_Keithley_2400 branch January 31, 2017 10:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Tom-Willemsen Tom-Willemsen Tom-Willemsen left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@AdrianPotter @Tom-Willemsen @John-Holt-Tessella