Add symlink parameter #468
Conversation
…r improved symlink handling
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #468 +/- ##
=======================================
- Coverage 72.5% 72.5% -0.1%
=======================================
Files 49 49
Lines 3635 3634 -1
=======================================
- Hits 2639 2637 -2
- Misses 996 997 +1 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
…e imports across the registry modules
|
I have fixed the changes you mentioned above and I manually tested the command with a symbolic link folder, and it worked flawlessly. Please let me know if there are any other major changes needed; otherwise, I’ll proceed with writing the tests! @lquerel |
@Leo6Leo Sounds good. Let me know when you are ready for the next review. Thanks |
|
@lquerel The tests have been added, the PR should be ready for review imo, PTAL, thanks! |
| @@ -402,4 +421,101 @@ mod tests { | |||
|
|
|||
| assert_eq!(rust_files, expected_rust_files); | |||
| } | |||
|
|
|||
| #[test] | |||
| fn test_registry_generate_with_symbolic_link_cases() { | |||
There was a problem hiding this comment.
The main issue I’m facing is having a unit test in the main crate that depends on test data from another crate (crates/weaver_codegen_test). I think it would be better to have this symbolic link in the data directory of the main crate to avoid this kind of dependency, which might cause problems at some point.
There was a problem hiding this comment.
Thank you for pointing that out! The reason I created the symbolic link in crates/weaver_codegen_test is because, based on the function test_registry_generate, it appears to rely on the registry located in crates/weaver_codegen_test/semconv_registry/.
I think this indicates that the test depends on the weaver_codegen_test crate to access the registry. To ensure the required dependencies are properly linked, I created the symbolic link there to align with this structure.
But I have created the commit to reflect the suggestion you recommend, PTAL f985907 @lquerel
fixes #441
Summary of the change I have made:
Added an option to follow symbolic links when loading the registry in various parts of the codebase. The primary change is the addition of a
follow_symlinksparameter, which is passed through multiple functions and structures to control this behavior.Key changes include:
follow_symlinksparam asWeaverArgacrossSchemaResolver,SnippetGenerator, andResolvedSemconvRegistryto enable optional symbolic link following during registry operations.-The
WeaverArgsstruct impacted various commands (check,generate,resolve,update-markdown,stats) to include and handle thefollow_symlinksoption.SchemaResolverto configurewalkdir::WalkDirwith thefollow_symlinksparameter and use thefollow_links function.follow_symlinks= false.The ideal outcome would be:
After running the command
weaver will read symbolic linked folders under
sementic-conventions/modeland generate the artifacts.PS: I try to raise this PR first to get some early feedback. As I am new to Rust and Otel, I am open to suggestions and feedback.