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

DOC: specify k2700 cpts are for ranges #1174

Merged
merged 2 commits into from
Oct 11, 2023
Merged

DOC: specify k2700 cpts are for ranges #1174

merged 2 commits into from
Oct 11, 2023

Conversation

KaushikMalapati
Copy link
Contributor

@KaushikMalapati KaushikMalapati commented Oct 9, 2023

Description

I added 'range' to the doc attributes and names of the components dcv, acv, dci, and aci of the K2700 class.

Motivation and Context

I originally thought that the pvs associated with these components were to readback the values in their names, i.e. GetDCV would read the direct current voltage, but they actually get the range configured for each measurement. You use :Reading to get the actually measurement.

How Has This Been Tested?

N/A

Where Has This Been Documented?

New upcoming release notes

Pre-merge checklist

  • Code works interactively
  • Code contains descriptive docstrings, including context and API
  • New/changed functions and methods are covered in the test suite where possible
  • Test suite passes locally
  • Test suite passes on GitHub Actions
  • Ran docs/pre-release-notes.sh and created a pre-release documentation page
  • Pre-release docs include context, functional descriptions, and contributors as appropriate

doc='DC current')
aci = Cpt(EpicsSignalRO, ":GetACI", kind="normal",
doc='AC current')
dcv_range = Cpt(EpicsSignalRO, ":GetDCV", kind="normal",
Copy link
Contributor

Choose a reason for hiding this comment

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

So this is technically a sort of "API Break", since we're changing the name of a component on this device. If there are older scripts that expect K2700.dcv, they won't be able to find that name any longer.

Practically, if you know you're the only one using this device and you won't break others' usage of the device, this is fine. I just want to make sure this is the case.

If it were me I'd update the documentation alone, but that may be out of an abundance of caution

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the clarification, release notes have been updated to note the API breaks. I recently added this device class and there aren't any happi entries using it other than one I made so it should be safe to change the component names.

Copy link
Contributor

@tangkong tangkong left a comment

Choose a reason for hiding this comment

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

LTGM, thanks for keeping the docs clear 👍

@tangkong tangkong merged commit 12cb7d7 into pcdshub:master Oct 11, 2023
9 checks passed
@ZLLentz ZLLentz mentioned this pull request Oct 16, 2023
@KaushikMalapati KaushikMalapati deleted the k2700 branch October 18, 2023 22:01
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.

2 participants