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

121 dodal integration #171

Merged
merged 24 commits into from
May 15, 2023
Merged

121 dodal integration #171

merged 24 commits into from
May 15, 2023

Conversation

abbiemery
Copy link
Contributor

Fixes #121 .

Needs to have this dodal pr to be merged to work, otherwise the devices that use other devices in the example start up will not work.

It also fails on the 3.8 tests, so maybe we remove the support for that first.

@abbiemery abbiemery marked this pull request as draft May 4, 2023 15:32
@callumforrester
Copy link
Contributor

The CI should be fixed if you rebase on main

Copy link
Contributor

@callumforrester callumforrester left a comment

Choose a reason for hiding this comment

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

Looking good! Only a few small items.

src/blueapi/config.py Outdated Show resolved Hide resolved
src/blueapi/config.py Show resolved Hide resolved
src/blueapi/core/context.py Outdated Show resolved Hide resolved
src/blueapi/core/context.py Outdated Show resolved Hide resolved
src/blueapi/startup/example.py Show resolved Hide resolved
@codecov
Copy link

codecov bot commented May 5, 2023

Codecov Report

Merging #171 (7b03b80) into main (76a4e02) will decrease coverage by 0.04%.
The diff coverage is 88.88%.

❗ Current head 7b03b80 differs from pull request most recent head c4ef315. Consider uploading reports for the commit c4ef315 to get more accurate results

@@            Coverage Diff             @@
##             main     #171      +/-   ##
==========================================
- Coverage   87.39%   87.36%   -0.04%     
==========================================
  Files          40       40              
  Lines        1079     1092      +13     
==========================================
+ Hits          943      954      +11     
- Misses        136      138       +2     
Impacted Files Coverage Δ
src/blueapi/service/handler.py 69.76% <0.00%> (ø)
src/blueapi/core/context.py 93.54% <86.66%> (-1.91%) ⬇️
src/blueapi/config.py 96.36% <100.00%> (+0.61%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@abbiemery
Copy link
Contributor Author

@callumforrester I made the changes you requested. I also added a better test case for the context which will cause the CI to fail untill the make_all_devices changes are implemented. Either by dodal or if we choose to migrate the method to the blueapi utils.

pyproject.toml Outdated Show resolved Hide resolved
@abbiemery abbiemery force-pushed the 121_dodal_integration branch from 946e188 to adac983 Compare May 11, 2023 16:09
@abbiemery abbiemery marked this pull request as ready for review May 11, 2023 16:15
@abbiemery
Copy link
Contributor Author

@callumforrester Done the last things you pointed out and rebased. The tests will still fail until the dodal updates are merged. I'm just waiting on a review for that.

Copy link
Contributor

@callumforrester callumforrester left a comment

Choose a reason for hiding this comment

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

Provisionally approving pending dodal PR merge. Only minor comments now. Some docstrings on new classes/methods would also be good if you have time.

src/blueapi/config.py Outdated Show resolved Hide resolved
@abbiemery abbiemery merged commit a90b53b into main May 15, 2023
@DiamondJoseph DiamondJoseph deleted the 121_dodal_integration branch September 5, 2024 10:32
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.

Dodal integration
3 participants