Skip to content

feat: Insert documents into Tiled #755

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Conversation

DiamondJoseph
Copy link
Contributor

No description provided.

pyproject.toml Outdated
@@ -95,7 +99,8 @@ addopts = """
--ignore=src/blueapi/startup
"""
# https://iscinumpy.gitlab.io/post/bound-version-constraints/#watch-for-warnings
filterwarnings = ["error", "ignore::DeprecationWarning"]
# Unignore UserWarning after Pydantic warning removed from bluesky/bluesky and release
Copy link
Contributor Author

Choose a reason for hiding this comment

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

bluesky/bluesky#1867

  • a release of bluesky

Copy link
Contributor

Choose a reason for hiding this comment

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

Should: Include issue link in a comment in the file

@DiamondJoseph DiamondJoseph marked this pull request as ready for review January 9, 2025 15:44
Copy link

codecov bot commented Jan 9, 2025

Codecov Report

Attention: Patch coverage is 68.75000% with 5 lines in your changes missing coverage. Please review.

Project coverage is 94.31%. Comparing base (4d2d6e9) to head (beeeb2f).

Files with missing lines Patch % Lines
src/blueapi/service/interface.py 58.33% 5 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #755      +/-   ##
==========================================
- Coverage   94.48%   94.31%   -0.17%     
==========================================
  Files          41       41              
  Lines        2555     2570      +15     
==========================================
+ Hits         2414     2424      +10     
- Misses        141      146       +5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

pyproject.toml Outdated
@@ -95,7 +99,8 @@ addopts = """
--ignore=src/blueapi/startup
"""
# https://iscinumpy.gitlab.io/post/bound-version-constraints/#watch-for-warnings
filterwarnings = ["error", "ignore::DeprecationWarning"]
# Unignore UserWarning after Pydantic warning removed from bluesky/bluesky and release
filterwarnings = ["error", "ignore::DeprecationWarning", "ignore::UserWarning"]
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any way to only ignore warnings from the tests in question? Otherwise we may end up removing this in a few months time only to find the tests blowing up in a bunch of other places?

@@ -48,6 +50,19 @@ def worker() -> TaskWorker:
return worker


@cache
def tiled_inserter():
Copy link
Contributor

Choose a reason for hiding this comment

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

Should:

Suggested change
def tiled_inserter():
def tiled_inserter() -> TiledClient:

...or whatever it's called

pyproject.toml Outdated
@@ -95,7 +99,8 @@ addopts = """
--ignore=src/blueapi/startup
"""
# https://iscinumpy.gitlab.io/post/bound-version-constraints/#watch-for-warnings
filterwarnings = ["error", "ignore::DeprecationWarning"]
# Unignore UserWarning after Pydantic warning removed from bluesky/bluesky and release
Copy link
Contributor

Choose a reason for hiding this comment

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

Should: Include issue link in a comment in the file

@DiamondJoseph DiamondJoseph requested a review from a team as a code owner July 3, 2025 09:38
@DiamondJoseph DiamondJoseph changed the title Minimal configuration for inserting documents into Tiled feat: Insert documents into Tiled Jul 3, 2025
Copy link
Contributor

@tpoliaw tpoliaw left a comment

Choose a reason for hiding this comment

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

Comment is mostly pedantry - looks good otherwise

@@ -148,6 +157,9 @@ def setup(config: ApplicationConfig) -> None:
)

stomp_client()
if config.tiled.enabled:
Copy link
Contributor

Choose a reason for hiding this comment

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

Would this check be better in the tiled_writer method (which then returns TiledWriter | None) so it's not relying on the config being checked externally? This could then be

if writer := tiled_writer():
    context().run_engine.subscribe(writer)

Maybe need to make it tiled_writer(TiledConfig) -> TiledWriter | None so that the caching still works if the config ever changes (does it?).

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