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

Make database file optional for EpicsAdapter #134

Merged
merged 3 commits into from
Jul 13, 2023

Conversation

joeshannon
Copy link
Contributor

No description provided.

@codecov
Copy link

codecov bot commented Jun 29, 2023

Codecov Report

Merging #134 (6dc6cd4) into master (6483aac) will increase coverage by 0.74%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #134      +/-   ##
==========================================
+ Coverage   92.48%   93.23%   +0.74%     
==========================================
  Files          45       45              
  Lines        1225     1226       +1     
==========================================
+ Hits         1133     1143      +10     
+ Misses         92       83       -9     
Impacted Files Coverage Δ
src/tickit/adapters/epicsadapter/adapter.py 100.00% <100.00%> (+12.24%) ⬆️

... and 1 file with indirect coverage changes

Copy link
Collaborator

@abbiemery abbiemery left a comment

Choose a reason for hiding this comment

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

Seams okay to me, would probably be worth putting a test in for the different cases. Also what do you think about writing an epics adapter for one of the example devices, maybe the amplifier? It would be without a db and could be run with the work rose is doing for system tests.

@joeshannon
Copy link
Contributor Author

Yeah that sounds like a good idea. Will take a look at that tomorrow.

@joeshannon joeshannon force-pushed the epics-optional-db-file branch from 388f84c to 6fc4afa Compare July 11, 2023 14:56
Copy link
Contributor Author

@joeshannon joeshannon left a comment

Choose a reason for hiding this comment

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

These two tests should maintain the coverage but they could possibly be improved.

@abbiemery abbiemery self-requested a review July 11, 2023 16:27
Copy link
Collaborator

@abbiemery abbiemery left a comment

Choose a reason for hiding this comment

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

After some discussion, we think to just ignore the types on the mocked functions and it should be happy.

tests/adapters/test_epicsadapter/test_epics_adapter.py Outdated Show resolved Hide resolved
@joeshannon joeshannon force-pushed the epics-optional-db-file branch from 6fc4afa to f4de9b8 Compare July 12, 2023 14:26
Copy link
Collaborator

@abbiemery abbiemery left a comment

Choose a reason for hiding this comment

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

LGTM,

@joeshannon joeshannon force-pushed the epics-optional-db-file branch from f4de9b8 to 4779811 Compare July 13, 2023 09:16
This is to make clear that the method is concerned with loading an
actual db file.
@joeshannon joeshannon force-pushed the epics-optional-db-file branch from 4779811 to 6dc6cd4 Compare July 13, 2023 09:20
@joeshannon joeshannon merged commit 94297cb into master Jul 13, 2023
@joeshannon joeshannon deleted the epics-optional-db-file branch July 13, 2023 09:23
joeshannon added a commit to DiamondLightSource/tickit-devices that referenced this pull request Jul 13, 2023
The db_file parameter has been changed to an optional parameter.

See DiamondLightSource/tickit#134
abbiemery pushed a commit to DiamondLightSource/tickit-devices that referenced this pull request Jul 25, 2023
The db_file parameter has been changed to an optional parameter.

See DiamondLightSource/tickit#134
abbiemery pushed a commit to DiamondLightSource/tickit-devices that referenced this pull request Jul 25, 2023
The db_file parameter has been changed to an optional parameter.

See DiamondLightSource/tickit#134
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