Skip to content

Conversation

@Ryosuke839
Copy link
Contributor

@Ryosuke839 Ryosuke839 commented Oct 30, 2025

Elementary shortens model name in Slack alerts. However, it will only show model version like v2 when tests were against versioned models https://docs.getdbt.com/docs/mesh/govern/model-versions.
(Note: Slack alert message is generated in

@property
def model(self) -> Optional[str]:
if not self.model_unique_id:
return None
return get_shortened_model_name(self.model_unique_id)
@property
def report_url(self) -> Optional[str]:
return self.alerts[0].report_url
@property
def summary(self) -> str:
return (
f"{self.model}: {len(self.alerts)} issues detected"
if self.model
else f"{len(self.alerts)} Issues detected"
)
)

image

This PR changes shortening logic so that shortened model name will consist of both model base name and model version like model.v2.

Summary by CodeRabbit

  • Bug Fixes

    • Improved shortening of versioned model names so both the model identifier and its version are displayed when applicable (e.g., four-part names now show the last two segments).
  • Tests

    • Added unit tests validating model-name shortening across edge cases: None, single-part, three-part, and four-part inputs.

@github-actions
Copy link
Contributor

👋 @Ryosuke839
Thank you for raising your pull request.
Please make sure to add tests and document all user-facing changes.
You can do this by editing the docs files in this pull request.

@coderabbitai
Copy link

coderabbitai bot commented Oct 30, 2025

Walkthrough

The get_shortened_model_name function in elementary/utils/models.py was changed to return the last two components for four-part versioned model names; unit tests were added in tests/unit/utils/test_models.py to validate behavior for None, single-part, three-part, and four-part inputs.

Changes

Cohort / File(s) Summary
Function Logic Update
elementary/utils/models.py
Updated get_shortened_model_name to split model paths with maxsplit=2 and return the last segment, causing four-part versioned names to yield the final two components joined by a dot.
Test Coverage
tests/unit/utils/test_models.py
Added unit tests covering None input, single-part names, three-part names (shorten to final part), and four-part versioned names (shorten to final two parts, e.g., models.foo.bar.v2bar.v2).

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

  • Files to pay attention to:
    • elementary/utils/models.py — verify maxsplit=2 behavior and edge-case handling (None, empty strings).
    • tests/unit/utils/test_models.py — ensure tests correctly assert expected shortened outputs and cover intended cases.

Poem

🐰 I hopped through paths both long and sly,

Split them neat with just one try,
Four parts now curl to two with glee,
Tests clap paws — it's working, see! 🥕✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The pull request title "Correctly shorten versioned model names" directly corresponds to the main change in the changeset. The modification to the get_shortened_model_name function now returns the last two path components (e.g., "bar.v2") instead of just the final component for versioned models, which aligns precisely with the title's intent to correctly shorten such names. The title is concise, specific, and clearly communicates the primary objective without being vague or misleading.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 28b7525 and 3ee37b3.

📒 Files selected for processing (1)
  • tests/unit/utils/test_models.py (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • tests/unit/utils/test_models.py

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between ba986eb and 28b7525.

📒 Files selected for processing (2)
  • elementary/utils/models.py (1 hunks)
  • tests/unit/utils/test_models.py (1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/unit/utils/test_models.py (1)
elementary/utils/models.py (1)
  • get_shortened_model_name (1-6)
🪛 Flake8 (7.3.0)
tests/unit/utils/test_models.py

[error] 19-19: redefinition of unused 'test_shorten_1_part' from line 9

(F811)

🪛 Ruff (0.14.2)
tests/unit/utils/test_models.py

19-19: Redefinition of unused test_shorten_1_part from line 9

(F811)

🔇 Additional comments (1)
elementary/utils/models.py (1)

5-6: Logic correctly implements versioned model name shortening with full backward compatibility.

Verification confirms the implementation works as intended:

  • All unit tests pass (4 test cases covering None, 1-part, 3-part, and 4-part model names)
  • Usage in elementary/monitor/alerts/alerts_groups/grouped_by_table.py is compatible with the updated behavior
  • No breaking changes to existing code

@Ryosuke839 Ryosuke839 temporarily deployed to elementary_test_env October 30, 2025 06:56 — with GitHub Actions Inactive
@NoyaOffer
Copy link
Contributor

Hey @Ryosuke839 ! Thank you for your contribution, appreciate it.
We're taking a look and hope to approve soon 😄

@GuyEshdat GuyEshdat merged commit f15cb18 into elementary-data:master Oct 30, 2025
5 checks passed
@NoyaOffer
Copy link
Contributor

@Ryosuke839 Thanks again for the contribution!
We're adding a few more fixes and features before the next version release, you can use Master for now 😄

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