- Notifications
You must be signed in to change notification settings - Fork1.7k
Ruby: add support for extracting overlay databases#19684
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
33c2550 tofb89f22CompareThere was a problem hiding this 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 PR adds support for overlay database extraction in Ruby by introducing a newdatabaseMetadata relation and path‐prefix transformation logic.
- Introduces a
--add-metadata-relationflag to the shared extractor generator and implementscreate_database_metadata(). - Adds
PathTransformerandnormalize_and_transform_pathin the shared extractor for prefix rewrites. - Updates the Ruby extractor to skip unchanged files, load overlay changes, apply path transformations, and emit an empty base‐metadata file.
Reviewed Changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| shared/tree-sitter-extractor/src/generator/mod.rs | Addedadd_metadata_relation flag andcreate_database_metadata |
| shared/tree-sitter-extractor/src/file_paths.rs | ImplementedPathTransformer,load_path_transformer, and path normalization with transformation |
| shared/tree-sitter-extractor/src/extractor/*.rs | Propagated transformer through file population and trap writing |
| ruby/ql/lib/ruby.dbscheme | Added newdatabaseMetadata relation definition |
| config/dbscheme-fragments.json | Registered theDatabase metadata fragment |
| ruby/extractor/src/extractor.rs | Added overlay‐change filtering, transformer usage, and metadata output |
| ruby/extractor/src/generator.rs | Enabled metadata relation in the Ruby generator |
Comments suppressed due to low confidence (1)
ruby/extractor/src/extractor.rs:347
- [nitpick] New
get_overlay_changed_fileslogic andload_path_transformerare not covered by existing tests. Consider adding unit tests for parsing the JSON changes file and path transformation behavior.
fn get_overlay_changed_files() -> Option<HashSet<PathBuf>> {Uh oh!
There was an error while loading.Please reload this page.
ruby/extractor/src/extractor.rs Outdated
| .map(|change| { | ||
| change | ||
| .as_str() | ||
| .map(|s| PathBuf::from(s).canonicalize().ok()) | ||
| .flatten() | ||
| }) | ||
| .collect() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Collecting an iterator ofOption<PathBuf> will not produceHashSet<PathBuf>. You should usefilter_map to dropNone entries and wrap the resultingHashSet inSome(...) before returning.
| .map(|change|{ | |
| change | |
| .as_str() | |
| .map(|s|PathBuf::from(s).canonicalize().ok()) | |
| .flatten() | |
| }) | |
| .collect() | |
| .filter_map(|change|{ | |
| change | |
| .as_str() | |
| .and_then(|s|PathBuf::from(s).canonicalize().ok()) | |
| }) | |
| .collect::<HashSet<PathBuf>>() | |
| .into() |
Copilot uses AI. Check for mistakes.
576f0d2 to4029b89Compare@aibaars do you have capacity to review this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Looks fine to me. A few comments though. I think it would be fine to add the new features to all extractors .
| @@ -32,6 +33,16 @@ pub fn generate( | |||
| writeln!(dbscheme_writer, include_str!("prefix.dbscheme"))?; | |||
| // Eventually all languages will have the metadata relation (for overlay support), at which | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think we can just shove the metadata tables directly inprefix.dbscheme. An unused table shouldn't hurt.
Uh oh!
There was an error while loading.Please reload this page.
| @@ -212,7 +212,7 @@ impl TrapFile { | |||
| ); | |||
| } | |||
| pub fn emit_file(&mut self, absolute_path: &Path) -> Label<generated::File> { | |||
| let untyped = extractor::populate_file(&mut self.writer, absolute_path); | |||
| let untyped = extractor::populate_file(&mut self.writer, absolute_path, None); | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I would just add support for path transformers to the Rust extractor.
| @@ -15,7 +15,7 @@ impl Archiver { | |||
| } | |||
| fn try_archive(&self, source: &Path) -> std::io::Result<()> { | |||
| let dest = file_paths::path_for(&self.root, source, ""); | |||
| let dest = file_paths::path_for(&self.root, source, "", None); | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I would just add support for path transformers to the Rust extractor.
Uh oh!
There was an error while loading.Please reload this page.
| .filter(|line| !line.is_empty()) | ||
| .collect::<Vec<String>>(); | ||
| if lines.len() != 2 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
How hard would it be to just implement path transformers completely?
4029b89 tofa24ee6CompareI've force-pushed a couple of the suggested fixes. Regarding the others:
|
This is required for overlay support.
Supports only a minimal subset of the project layout specification;enough to work with the transformers produced by the CLI when buildingan overlay database.
fa24ee6 to665df4bCompareThe previous implementation returned None if any of the paths in thechanges JSON couldn't be canonicalized. This could happen for files thatwere deleted in the diff. Now, it just ignores paths for whichcanonicalize() fails.
1bbba2f intomainUh oh!
There was an error while loading.Please reload this page.
There will need to be followup changes on the QL side before querying will work, but this is enough to be able to build an overlay database for Ruby.