Skip to content

Conversation

@lmolkova
Copy link
Member

No description provided.

@lmolkova lmolkova requested a review from a team as a code owner September 10, 2025 01:42
@lmolkova lmolkova requested a review from Copilot September 10, 2025 01:44
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This pull request adds validation to ensure template patterns match existing files when creating a TemplateEngine. The change modifies the TemplateEngine::new method to return a Result and fail early if any template pattern doesn't match any files.

  • Changed TemplateEngine::new to return Result<Self, Error> instead of Self
  • Added validation logic to check template patterns against available files
  • Updated all call sites to handle the new return type with proper error handling

Reviewed Changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
crates/weaver_forge/src/lib.rs Modified TemplateEngine::new to validate templates and return Result, added test for validation
src/registry/update_markdown.rs Updated to handle new Result return type from TemplateEngine::new
src/registry/live_check.rs Updated to handle new Result return type from TemplateEngine::new
src/registry/generate.rs Updated to handle new Result return type from TemplateEngine::new
src/registry/diff.rs Updated to handle new Result return type from TemplateEngine::new
src/main.rs Updated to handle new Result return type from TemplateEngine::new and improved debug logging
crates/weaver_semconv_gen/src/lib.rs Updated to handle new Result return type from TemplateEngine::new
crates/weaver_forge/templates/wrong_config/weaver.yaml Added test configuration with non-existent template
crates/weaver_forge/templates/wrong_config/exists.j2 Added test template file
crates/weaver_forge/templates/whitespace_control/weaver.yaml Removed unused template configurations
crates/weaver_codegen_test/templates/registry/rust/weaver.yaml Removed unused template configurations
crates/weaver_codegen_test/build.rs Updated to handle new Result return type from TemplateEngine::new

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@codecov
Copy link

codecov bot commented Sep 10, 2025

Codecov Report

❌ Patch coverage is 94.11765% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 78.0%. Comparing base (e90fdf5) to head (c486413).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
crates/weaver_forge/src/lib.rs 94.1% 1 Missing ⚠️
Additional details and impacted files
@@          Coverage Diff          @@
##            main    #928   +/-   ##
=====================================
  Coverage   77.9%   78.0%           
=====================================
  Files         76      76           
  Lines       5980    6002   +22     
=====================================
+ Hits        4661    4684   +23     
+ Misses      1319    1318    -1     

☔ 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.

Copy link
Contributor

@jerbly jerbly left a comment

Choose a reason for hiding this comment

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

We should have a changelog entry. Otherwise LGTM.

Copy link
Contributor

@lquerel lquerel left a comment

Choose a reason for hiding this comment

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

I made a minor suggestion, otherwise LGTM. Thanks

@jsuereth jsuereth enabled auto-merge (squash) September 15, 2025 12:44
@jsuereth jsuereth merged commit cf351e3 into open-telemetry:main Sep 16, 2025
20 checks passed
@github-project-automation github-project-automation bot moved this from Next Release to Done in OTel Weaver Project Sep 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

4 participants