Skip to content

Mark ambiguous run summary recommendations#1926

Open
snoopuppy582 wants to merge 4 commits into
pydantic:mainfrom
snoopuppy582:fix/run-summary-special-case-note
Open

Mark ambiguous run summary recommendations#1926
snoopuppy582 wants to merge 4 commits into
pydantic:mainfrom
snoopuppy582:fix/run-summary-special-case-note

Conversation

@snoopuppy582
Copy link
Copy Markdown

Fixes #1296

Summary

  • Mark ambiguous logfire run recommendations (requests, sqlite3, urllib) with *.
  • Add a short note explaining that these packages can be detected from environment availability even when the app does not use them directly.
  • Keep normal recommendation/install-command behavior unchanged.

Tests

  • uv run pytest tests/test_cli.py::test_get_recommendation_texts tests/test_cli.py::test_get_recommendation_texts_marks_ambiguous_packages -q
  • uv run ruff check logfire/_internal/cli/run.py tests/test_cli.py
  • uv run ruff format --check logfire/_internal/cli/run.py tests/test_cli.py

Notes

Copy link
Copy Markdown
Contributor

@cubic-dev-ai cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

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

No issues found across 2 files

Confidence score: 5/5

  • Automated review surfaced no issues in the provided summaries.
  • No files require special attention.

@snoopuppy582
Copy link
Copy Markdown
Author

I pushed a small snapshot-spacing follow-up after CI.

The test matrix was failing in tests/test_cli.py::test_inspect; the rendered Rich panel line for the new urllib* note had one fewer padding space than my snapshot expected.

Local checks after the follow-up:

  • uv run pytest tests/test_cli.py::test_get_recommendation_texts tests/test_cli.py::test_get_recommendation_texts_marks_ambiguous_packages -q -> 2 passed
  • uv run ruff check logfire/_internal/cli/run.py tests/test_cli.py -> passed
  • uv run ruff format --check logfire/_internal/cli/run.py tests/test_cli.py -> passed
  • git diff --check -> passed

@snoopuppy582
Copy link
Copy Markdown
Author

CI update after the snapshot-spacing follow-up:

  • lint, docs, test on Pyodide, Cubic review, and the Python 3.10-3.14 matrix jobs passed.
  • The remaining real test failure is only test on Python 3.9, pydantic 2, otel 1, in tests/otel_integrations/test_celery.py::test_instrument_celery.
  • That job reports 1586 passed, 41 skipped, 2 xfailed, and one Celery span assertion failure where the observed span is run/tasks.say_hello rather than the expected apply_async/tasks.say_hello.

This looks unrelated to this PR's CLI recommendation text/snapshot change. I tried to rerun the failed job, but GitHub requires repository admin rights for reruns on this workflow.

@codecov
Copy link
Copy Markdown

codecov Bot commented May 14, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

Comment thread tests/test_cli.py Outdated
@snoopuppy582
Copy link
Copy Markdown
Author

Thanks, applied the wording suggestion in 450535bc.

Changes:

  • Changed ambiguous recommendation markers from urllib* to urllib [*].
  • Updated the note wording to: [*] ... may not actually be used by your app, in which case you can ignore ....
  • Updated the focused recommendation-text assertions and the inspect snapshot.

Verified locally:

  • uv run pytest tests/test_cli.py::test_get_recommendation_texts tests/test_cli.py::test_get_recommendation_texts_marks_ambiguous_packages -q -> 2 passed
  • uv run ruff check logfire/_internal/cli/run.py tests/test_cli.py -> passed
  • uv run ruff format --check logfire/_internal/cli/run.py tests/test_cli.py -> passed
  • git diff --check -> passed

Note: tests/test_cli.py::test_inspect still has the known Windows local Rich/box-drawing snapshot rendering difference, so I kept the repository snapshot aligned with the GitHub/reviewer suggestion rather than the local Windows mojibake output.

Copy link
Copy Markdown
Contributor

@devin-ai-integration devin-ai-integration Bot left a comment

Choose a reason for hiding this comment

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

Devin Review found 1 potential issue.

View 3 additional findings in Devin Review.

Open in Devin Review

Comment on lines +265 to +269
def _format_package_list(package_names: list[str]) -> str:
quoted_names = [f'`{name}`' for name in package_names]
if len(quoted_names) == 1:
return quoted_names[0]
return f'{", ".join(quoted_names[:-1])}, and {quoted_names[-1]}'
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 _format_package_list produces incorrect grammar for 2-item lists

For a 2-item list like ['requests', 'sqlite3'], _format_package_list returns `requests`, and `sqlite3` (with a spurious comma before "and"). English grammar for 2-item lists is "X and Y" (no comma); the Oxford comma style "X, and Y" is only correct for 3+ items. This is reachable when exactly 2 of the 3 AMBIGUOUS_RECOMMENDATION_PACKAGES appear in recommendations (e.g. the user already has opentelemetry-instrumentation-urllib installed, so only requests and sqlite3 are flagged).

Suggested change
def _format_package_list(package_names: list[str]) -> str:
quoted_names = [f'`{name}`' for name in package_names]
if len(quoted_names) == 1:
return quoted_names[0]
return f'{", ".join(quoted_names[:-1])}, and {quoted_names[-1]}'
def _format_package_list(package_names: list[str]) -> str:
quoted_names = [f'`{name}`' for name in package_names]
if len(quoted_names) == 1:
return quoted_names[0]
if len(quoted_names) == 2:
return f'{quoted_names[0]} and {quoted_names[1]}'
return f'{', '.join(quoted_names[:-1])}, and {quoted_names[-1]}'
Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

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.

logfire run always suggests requests, sqlite3, urllib

2 participants