successful build #1
Conversation
WalkthroughManifests moved from a workspace model to explicit per-crate dependencies and path refs; crate editions set to 2021; new binaries and example crate added; crates-io patch removed; procedural macro replaced by a macro invocation; CLI now scans and processes crates with backup utilities, per-crate configs, patches, docs, and examples. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User as CLI (user)
participant Main as split-decls-rs
participant FS as FileSystem
participant Proc as process_crate
participant Patch as PatchApplier
Note over Main,FS: parse args (--dry-run, --verbose, path)
User->>Main: invoke with path + flags
Main->>FS: stat path (is Cargo.toml present?)
alt single crate
FS-->>Main: Cargo.toml present
Main->>Proc: process_crate(crate_path, global_config, dry_run)
else scan root
Main->>FS: walk directory tree (WalkDir)
loop each discovered Cargo.toml
FS-->>Main: crate_path
Main->>FS: read Cargo.toml (check [package])
alt real crate and not self
Main->>Proc: process_crate(crate_path, per_crate_config, dry_run)
else skip
Main-->>FS: skip virtual or self crate
end
end
end
Note over Proc,Patch: per-crate processing
Proc->>FS: write `.split-decls-config.toml` (per-crate)
Proc->>FS: read `src/lib.rs`
Proc->>Patch: apply_patches_to_syntax_tree(syntax_tree, PatchConfig)
Patch-->>FS: write modified files (or log on dry-run)
Proc-->>Main: done
Main-->>User: exit (status/messages)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
PR Compliance Guide 🔍Below is a summary of compliance checks for this PR:
Compliance status legend🟢 - Fully Compliant🟡 - Partial Compliant 🔴 - Not Compliant ⚪ - Requires Further Human Verification 🏷️ - Compliance label |
|||||||||||||||||||||||
PR Code Suggestions ✨Explore these optional code suggestions:
|
||||||||||||||
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
output/macros/Scalar.rs (1)
1-1: Unusual formatting in macro invocation.The spacing around
::,!, and;is non-idiomatic. If this is generated output, it's acceptable. If this is source code, consider runningrustfmtto normalize:async_graphql::scalar!(AssetId);Also note that
AssetIdis not defined or imported in this file. Verify this file is meant to be included elsewhere or thatAssetIdis in scope.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
Cargo.tomlbuild_helpers/cargo_toml_parts/Cargo.tomlcargo-toml-generator-macros/Cargo.tomlcargo-toml-generator-types/Cargo.tomloutput/macros/Scalar.rssplit-decls-types/Cargo.tomltest_config.toml
🔇 Additional comments (10)
cargo-toml-generator-macros/Cargo.toml (1)
12-12: LGTM!The migration from workspace-based to explicit version specification for
proc-macro2is consistent with the broader migration approach in this PR.test_config.toml (1)
1-8: Placeholder configuration looks fine.This appears to be scaffolding for future test configuration. Consider documenting the expected schema for
[patches]and[string_replacements]sections when they are populated.split-decls-types/Cargo.toml (2)
4-4: Edition downgrade is appropriate.Edition 2024 is still unstable. Switching to 2021 ensures compatibility with stable Rust toolchains.
7-9: Explicit dependency versions look good.The versions specified (
serde 1.0,toml 0.8,anyhow 1.0) are current and widely used. This aligns with the migration from workspace dependencies.cargo-toml-generator-types/Cargo.toml (1)
10-10: LGTM!Consistent migration from workspace-based to explicit version for
proc-macro2.build_helpers/cargo_toml_parts/Cargo.toml (2)
4-4: LGTM - Edition downgrade.Switching from unstable 2024 to stable 2021 edition is the right call for build compatibility.
7-9: Path dependencies and explicit version are consistent.The relative paths (
../../cargo-toml-generator-macros,../../cargo-toml-generator-types) correctly reference the crates at the repository root frombuild_helpers/cargo_toml_parts/.Cargo.toml (3)
4-4: Edition downgrade to 2021 is appropriate.Edition 2024 is not yet stable. Using 2021 ensures compatibility with stable Rust toolchains.
9-24: Dependency migration looks consistent.All dependencies have been migrated from workspace-based to explicit version specifications. The versions used are current and appropriate. Path dependencies reference crates at expected locations within the repository.
34-44: All declared binary source files exist at their specified paths.The three binaries (rustmacrodoc, testbuild, macro_analyzer) are properly configured in Cargo.toml with corresponding source files present.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
beginners_readme.md
🧰 Additional context used
🪛 markdownlint-cli2 (0.18.1)
beginners_readme.md
186-186: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
194-194: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
286-286: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (12)
example_project/Cargo.toml (2)
2-2: Consider aligning package name with directory name.The package name
example_cratediffers from the directory nameexample_project. While valid, aligning them (e.g., both asexample_project) would improve clarity and follow common conventions.🔎 Proposed fix
-name = "example_crate" +name = "example_project"
6-6: Empty dependencies section can be omitted.The
[dependencies]section is currently empty and can be removed until dependencies are actually needed.🔎 Proposed fix
- -[dependencies]patches/better_hello.rs (1)
1-3: Consider returning a string slice to avoid allocation.The function allocates a new
Stringon every call. If the greeting never changes, returning&'static strwould be more efficient.🔎 Proposed refactor
-pub fn hello() -> String { - "Hello, Amazing Universe!".to_string() +pub fn hello() -> &'static str { + "Hello, Amazing Universe!" }beginners_readme.md (1)
156-165: Minor grammar fix: use hyphenated compound adjective."backed up content" should be "backed-up content" when used as a compound adjective.
🔎 Proposed fix
-4. **Applies string replacements** to the backed up content +4. **Applies string replacements** to the backed-up contentsrc/backup_original_files.rs (2)
18-22: Consider adding error context tofs::writefor consistency.The
fs::renamecalls have descriptive.context()messages, but thefs::writecalls don't. Adding context would improve error diagnostics.🔎 Proposed fix
} else if !paths.lib_rs_path.exists() { // If neither lib.rs nor oldlib.rs exists, create an empty oldlib.rs - fs::write(&paths.old_lib_rs_path, "")?; + fs::write(&paths.old_lib_rs_path, "") + .context(format!("Failed to create empty {}", paths.old_lib_rs_path.display()))?; println!("No {} found, created empty {}", paths.lib_rs_path.display(), paths.old_lib_rs_path.display()); }
31-35: Same here: add error context tofs::write.🔎 Proposed fix
} else if !paths.build_rs_path.exists() { // If neither build.rs nor oldbuild.rs exists, create an empty oldbuild.rs - fs::write(&paths.old_build_rs_path, "")?; + fs::write(&paths.old_build_rs_path, "") + .context(format!("Failed to create empty {}", paths.old_build_rs_path.display()))?; println!("No {} found, created empty {}", paths.build_rs_path.display(), paths.old_build_rs_path.display()); }src/backup_original_cargo.rs (1)
18-22: Consider adding error context tofs::writefor consistency.Same pattern as
backup_original_files.rs- thefs::writecall lacks context whilefs::renamehas it.🔎 Proposed fix
} else if !paths.cargo_toml_path.exists() { // If no Cargo.toml exists and no backup exists, create an empty oldCargo.toml - fs::write(&paths.old_cargo_toml_path, "")?; + fs::write(&paths.old_cargo_toml_path, "") + .context(format!("Failed to create empty {}", paths.old_cargo_toml_path.display()))?; println!("No {} found, created empty {}", paths.cargo_toml_path.display(), paths.old_cargo_toml_path.display()); }src/process_crate.rs (1)
49-49: Remove stale commented-out code.This commented-out call to
generate_new_build_rsis superseded by the actual call at line 80. Remove it to avoid confusion.🔎 Proposed fix
generate_new_lib_rs(&paths, dry_run)?; - // generate_new_build_rs(&paths, dry_run)?; // We will modify this or remove it latersrc/main.rs (4)
10-32: Consider using a CLI parsing library.The manual argument parsing is fragile and lacks features like help messages, proper error handling, and validation. Consider using
clapfor a more robust and maintainable CLI interface.🔎 Example refactor using clap
+use clap::Parser; + +#[derive(Parser, Debug)] +#[command(name = "split-decls-rs")] +#[command(about = "Split Rust declarations tool", long_about = None)] +struct Args { + /// Target path (file or directory) + #[arg(default_value = "./")] + target: PathBuf, + + /// Run without modifying files + #[arg(long)] + dry_run: bool, + + /// Verbose output + #[arg(long)] + verbose: bool, +} + fn main() -> Result<()> { println!("Starting split-decls-rs tool..."); - let args: Vec<String> = std::env::args().collect(); - - // Parse command line arguments - let dry_run = args.contains(&"--dry-run".to_string()); - let verbose = args.contains(&"--verbose".to_string()); - - if dry_run { + let args = Args::parse(); + + if args.dry_run { println!("*** Running in DRY-RUN mode. No files will be modified. ***"); } - - // Get target directory from command line args - let target_path = if args.len() > 1 { - let mut target_arg = None; - for (i, arg) in args.iter().enumerate() { - if !arg.starts_with("--") && i > 0 { - target_arg = Some(arg.as_str()); - break; - } - } - target_arg.unwrap_or("./") - } else { - "./" - }; - - let target_path = PathBuf::from(target_path); - println!("Target path: {}", target_path.display()); + + println!("Target path: {}", args.target.display());
62-62: Move WalkDir import to module level.While function-scoped imports are valid, module-level imports are more conventional and easier to maintain.
+use walkdir::WalkDir; + fn main() -> Result<()> {Then remove line 62's import statement.
77-82: Consider moving struct definition to module level.The inline struct definition inside the function is valid but unconventional. Moving it to module level improves readability and follows Rust conventions.
🔎 Proposed refactor
At module level:
+#[derive(Debug, serde::Deserialize)] +struct MinimalCargoToml { + package: Option<toml::Table>, +} + fn main() -> Result<()> {Then remove the inline definition at lines 77-80.
64-66: Consider logging WalkDir errors instead of silently ignoring them.The
filter_map(|e| e.ok())pattern silently discards errors from WalkDir (e.g., permission denied, broken symlinks). This could cause crates to be silently skipped without user awareness.🔎 Proposed improvement
- for entry in WalkDir::new(root_path) - .into_iter() - .filter_map(|e| e.ok()) { + for entry in WalkDir::new(root_path).into_iter() { + let entry = match entry { + Ok(e) => e, + Err(err) => { + eprintln!("Warning: Failed to read directory entry: {}", err); + continue; + } + }; if entry.file_name() == "Cargo.toml" {
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
beginners_readme.mdexample_project/Cargo.tomlexample_project/src/lib.rsoutput/Cargo.tomloutput/wrapped-unimacro_derive/oldCargo.tomloutput/wrapped-unimacro_derive/src/oldlib.rspatches/better_hello.rssplit-decls-rs.tomlsrc/backup_original_cargo.rssrc/backup_original_files.rssrc/lib.rssrc/main.rssrc/patch_config.rssrc/process_crate.rs
✅ Files skipped from review due to trivial changes (1)
- output/Cargo.toml
🧰 Additional context used
🧬 Code graph analysis (2)
src/lib.rs (2)
src/process_crate.rs (1)
process_crate(14-83)src/backup_original_files.rs (1)
backup_original_files(6-37)
src/main.rs (1)
src/process_crate.rs (1)
process_crate(14-83)
🪛 LanguageTool
beginners_readme.md
[grammar] ~160-~160: Use a hyphen to join words.
Context: ...lies string replacements** to the backed up content 5. Splits code into indiv...
(QB_NEW_EN_HYPHEN)
🪛 markdownlint-cli2 (0.18.1)
beginners_readme.md
107-107: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
169-169: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
177-177: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (12)
patches/better_hello.rs (2)
5-7: LGTM!The
greetfunction uses the standard Rust formatting pattern and is implemented correctly.
1-7: Remove or properly integratepatches/better_hello.rs.This file contains unused public functions with no imports or calls in the codebase. The file is not part of the module system (no
mod patchesdeclaration) and is not referenced in Cargo.toml. Either integrate these functions into the main codebase if they're needed, or remove the file if it's outdated example/test code.If keeping these functions, add documentation comments explaining their purpose and usage.
Likely an incorrect or invalid review comment.
src/patch_config.rs (1)
32-40: LGTM!Adding
Defaultto the derive macro is appropriate here since all fields already have#[serde(default)]attributes and areVectypes that naturally support default initialization. This enables clean instantiation viaPatchConfig::default()as used inprocess_crate.rs.src/backup_original_files.rs (1)
1-3: LGTM!Imports are appropriate for the file's functionality.
example_project/src/lib.rs (1)
1-18: LGTM!Clean example library with simple public functions and a struct. This provides a good test case for demonstrating the split-decls-rs tool functionality.
Note: The AI summary mentions module exports (
process_crate,backup_original_files,backup_original_cargo) that don't appear in this file - those are insrc/lib.rs, notexample_project/src/lib.rs.src/backup_original_cargo.rs (1)
1-3: LGTM!Imports are appropriate for the backup functionality.
src/lib.rs (1)
29-31: LGTM!The new public module declarations properly expose the per-crate processing and backup utilities. This follows the existing module declaration pattern in the file.
src/process_crate.rs (3)
1-12: LGTM!The imports are well-organized and bring in all necessary dependencies for the per-crate processing workflow.
44-47: Placeholder acknowledged; consider documenting expected behavior.The comment clearly notes this is a "dummy patch config for now." When the full implementation is ready, ensure the actual
PatchConfig(with propergenerated_workspace_member,generated_workspace_dependency, etc.) is populated here instead of the default.
51-63: Potential parse failure whenoldlib.rsis empty.If
lib.rsdoesn't exist,backup_original_filescreates an emptyoldlib.rs. Reading and parsing this empty file at line 62 will succeed (empty file is valid Rust), but verify this is the intended behavior when no source file exists.split-decls-rs.toml (1)
1-14: Configuration is valid.The simplified configuration structure is correct. The patch file
patches/better_hello.rsexists in the repository as referenced.src/main.rs (1)
62-62: No action needed. Thewalkdirdependency is properly declared in Cargo.toml (version 2.5).
| ``` | ||
| my_project/ | ||
| ├── split-decls-rs.toml # Configuration | ||
| ├── patches/ # Your fixes | ||
| │ └── better_hello.rs | ||
| ├── target_crate/ # Project to modify | ||
| │ ├── Cargo.toml | ||
| │ └── src/ | ||
| │ └── lib.rs | ||
| └── output/ # Generated files | ||
| └── Cargo.toml | ||
| ``` |
There was a problem hiding this comment.
Add language identifier to fenced code block.
The directory structure code block lacks a language specification. Add text to satisfy markdown linting rules.
🔎 Proposed fix
-```
+```text
my_project/🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
107-107: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
In beginners_readme.md around lines 107 to 118, the fenced code block showing
the directory structure is missing a language identifier; update the opening
fence to include "text" (i.e., change ``` to ```text) so the code block is
explicitly marked as plain text for markdown linting.
| ``` | ||
| target_crate/ | ||
| ├── src/ | ||
| │ └── lib.rs | ||
| └── Cargo.toml | ||
| ``` | ||
|
|
||
| **After:** | ||
| ``` | ||
| target_crate/ | ||
| ├── src/ | ||
| │ ├── lib.rs # New gateway file | ||
| │ ├── oldlib.rs # Original backed up | ||
| │ └── decls/ # Split declarations | ||
| │ ├── target_crate_decls_hello.rs | ||
| │ ├── target_crate_decls_add.rs | ||
| │ └── _decl_module_invocation.rs | ||
| └── Cargo.toml | ||
| ``` |
There was a problem hiding this comment.
Add language identifiers to directory structure code blocks.
Both "Before" and "After" directory structure blocks lack language specifications.
🔎 Proposed fix
**Before:**
-```
+```text
target_crate/
├── src/
│ └── lib.rs
└── Cargo.toml **After:**
-```
+```text
target_crate/🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
169-169: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
177-177: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
In beginners_readme.md around lines 169 to 187, the "Before" and "After"
directory structure code blocks are missing language identifiers; update both
fenced code blocks to include a language tag such as "text" (e.g., change ``` to
```text) so the directory trees render consistently and preserve formatting.
| let target_path = PathBuf::from(target_path); | ||
| println!("Target path: {}", target_path.display()); | ||
|
|
||
| // Load configuration | ||
| let global_config_path = PathBuf::from("split-decls-rs.toml"); | ||
| let global_config = SplitDeclsConfig::load_from_file(&global_config_path) | ||
| .context("Failed to load global split-decls-rs config")?; | ||
| println!("Global config loaded: {:?}", global_config); | ||
|
|
||
| // --- Load generated Cargo.toml and extract workspace dependencies --- | ||
| let root_cargo_toml_content = fs::read_to_string(&root_cargo_toml_path) | ||
| .context(format!("Failed to read generated Cargo.toml from {}", root_cargo_toml_path.display()))?; | ||
|
|
||
| // Use the new CargoToml struct | ||
| let root_cargo_toml: CargoToml = toml::from_str(&root_cargo_toml_content) | ||
| .context(format!("Failed to parse generated Cargo.toml from {}", root_cargo_toml_path.display()))?; | ||
|
|
||
| // Populate workspace dependencies from the generated CargoToml | ||
| if let Some(workspace_section) = root_cargo_toml.workspace { | ||
| global_config.workspace_dependencies.extend(dep_to_toml_value_iter(workspace_section.workspace_dependencies)); | ||
|
|
||
| if verbose { | ||
| println!("Global config loaded: {:?}", global_config); | ||
| } | ||
| // Also extend with top-level dependencies, if any, for compatibility | ||
| global_config.workspace_dependencies.extend(dep_to_toml_value_iter(root_cargo_toml.dependencies)); | ||
| global_config.workspace_dependencies.extend(dep_to_toml_value_iter(root_cargo_toml.dev_dependencies)); | ||
| global_config.workspace_dependencies.extend(dep_to_toml_value_iter(root_cargo_toml.build_dependencies)); // Include build-dependencies as well | ||
|
|
||
| // Add dependencies from [patch] sections to workspace_dependencies | ||
| if let Some(patch_section) = root_cargo_toml.patch { | ||
| global_config.workspace_dependencies.extend(dep_to_toml_value_iter(patch_section.crates_io)); | ||
|
|
||
| // Check if target is a single crate or directory with multiple crates | ||
| if target_path.join("Cargo.toml").exists() { | ||
| // Single crate | ||
| println!("Processing single crate: {}", target_path.display()); | ||
| process_crate(&target_path, &global_config, dry_run)?; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Validate that the target path exists.
The code doesn't verify that target_path exists or is accessible before attempting to use it. This could lead to unclear error messages later in the processing pipeline.
🔎 Proposed validation check
let target_path = PathBuf::from(target_path);
println!("Target path: {}", target_path.display());
+// Validate target path exists
+if !target_path.exists() {
+ anyhow::bail!("Target path does not exist: {}", target_path.display());
+}
+
// Load configuration📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let target_path = PathBuf::from(target_path); | |
| println!("Target path: {}", target_path.display()); | |
| // Load configuration | |
| let global_config_path = PathBuf::from("split-decls-rs.toml"); | |
| let global_config = SplitDeclsConfig::load_from_file(&global_config_path) | |
| .context("Failed to load global split-decls-rs config")?; | |
| println!("Global config loaded: {:?}", global_config); | |
| // --- Load generated Cargo.toml and extract workspace dependencies --- | |
| let root_cargo_toml_content = fs::read_to_string(&root_cargo_toml_path) | |
| .context(format!("Failed to read generated Cargo.toml from {}", root_cargo_toml_path.display()))?; | |
| // Use the new CargoToml struct | |
| let root_cargo_toml: CargoToml = toml::from_str(&root_cargo_toml_content) | |
| .context(format!("Failed to parse generated Cargo.toml from {}", root_cargo_toml_path.display()))?; | |
| // Populate workspace dependencies from the generated CargoToml | |
| if let Some(workspace_section) = root_cargo_toml.workspace { | |
| global_config.workspace_dependencies.extend(dep_to_toml_value_iter(workspace_section.workspace_dependencies)); | |
| if verbose { | |
| println!("Global config loaded: {:?}", global_config); | |
| } | |
| // Also extend with top-level dependencies, if any, for compatibility | |
| global_config.workspace_dependencies.extend(dep_to_toml_value_iter(root_cargo_toml.dependencies)); | |
| global_config.workspace_dependencies.extend(dep_to_toml_value_iter(root_cargo_toml.dev_dependencies)); | |
| global_config.workspace_dependencies.extend(dep_to_toml_value_iter(root_cargo_toml.build_dependencies)); // Include build-dependencies as well | |
| // Add dependencies from [patch] sections to workspace_dependencies | |
| if let Some(patch_section) = root_cargo_toml.patch { | |
| global_config.workspace_dependencies.extend(dep_to_toml_value_iter(patch_section.crates_io)); | |
| // Check if target is a single crate or directory with multiple crates | |
| if target_path.join("Cargo.toml").exists() { | |
| // Single crate | |
| println!("Processing single crate: {}", target_path.display()); | |
| process_crate(&target_path, &global_config, dry_run)?; | |
| let target_path = PathBuf::from(target_path); | |
| println!("Target path: {}", target_path.display()); | |
| // Validate target path exists | |
| if !target_path.exists() { | |
| anyhow::bail!("Target path does not exist: {}", target_path.display()); | |
| } | |
| // Load configuration | |
| let global_config_path = PathBuf::from("split-decls-rs.toml"); | |
| let global_config = SplitDeclsConfig::load_from_file(&global_config_path) | |
| .context("Failed to load global split-decls-rs config")?; | |
| if verbose { | |
| println!("Global config loaded: {:?}", global_config); | |
| } | |
| // Check if target is a single crate or directory with multiple crates | |
| if target_path.join("Cargo.toml").exists() { | |
| // Single crate | |
| println!("Processing single crate: {}", target_path.display()); | |
| process_crate(&target_path, &global_config, dry_run)?; |
🤖 Prompt for AI Agents
In src/main.rs around lines 34 to 50, the code uses target_path without
verifying it exists or is accessible; add an early validation after creating
target_path: check that target_path.exists() (and optionally
target_path.is_dir() when expecting a directory) and attempt to canonicalize or
read metadata to ensure accessibility; if the check fails, return a clear error
(using anyhow::bail! or Err with context) describing that the provided target
path does not exist or is not accessible, so downstream processing doesn’t
produce confusing errors.
| let crate_name = crate_path | ||
| .file_name() | ||
| .and_then(|s| s.to_str()) | ||
| .context("Could not get crate name")?; | ||
|
|
||
| println!("Attempting to compose target build.rs from parts in: {}", target_build_rs_parts_dir.display()); | ||
| build_script_composer::compose_build_script_from_parts( | ||
| &target_build_rs_parts_dir, | ||
| &generated_target_build_rs_path, | ||
| )?; | ||
| // --- END NEW --- | ||
| // Skip self crate | ||
| if crate_name == "split-decls-rs" { | ||
| println!("Skipping self crate: split-decls-rs"); | ||
| continue; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/main.rs | sed -n '80,110p'Repository: meta-introspector/split-decls-rs
Length of output: 1167
🏁 Script executed:
grep -n "struct MinimalCargoToml" src/main.rs -A 5Repository: meta-introspector/split-decls-rs
Length of output: 383
🏁 Script executed:
grep -n "fn process_crate" src/main.rs -A 20Repository: meta-introspector/split-decls-rs
Length of output: 58
🏁 Script executed:
rg "fn process_crate" -A 15Repository: meta-introspector/split-decls-rs
Length of output: 2539
🏁 Script executed:
rg "fn setup_crate_paths" -A 20Repository: meta-introspector/split-decls-rs
Length of output: 1276
🏁 Script executed:
cat Cargo.toml | grep -A 2 "^\[package\]"Repository: meta-introspector/split-decls-rs
Length of output: 127
🏁 Script executed:
rg "package.get\(\"name\"\)" -A 2 -B 2Repository: meta-introspector/split-decls-rs
Length of output: 58
🏁 Script executed:
#!/bin/bash
# Find all Cargo.toml files and check if directory name matches package name
fd -t f 'Cargo.toml' -x sh -c '
dir_name=$(basename $(dirname "{}"))
pkg_name=$(grep "^name = " "{}" | head -1 | sed "s/name = \"\(.*\)\"/\1/")
if [ -n "$pkg_name" ] && [ "$dir_name" != "$pkg_name" ]; then
echo "Mismatch: Directory=$dir_name, Package=$pkg_name in {}"
fi
' | head -20Repository: meta-introspector/split-decls-rs
Length of output: 417
Self-crate detection relies on directory name, not actual package name.
The check at line 96 compares the directory name to "split-decls-rs", but the package name is an identifier used to refer to the package and as the default name of inferred lib and bin targets, which may differ from the directory name. The repository contains examples where this mismatch occurs (e.g., directory example_project with package name example_crate), creating two potential failure modes:
- False negative: If split-decls-rs is in a differently-named directory, it won't be skipped
- False positive: If an unrelated crate is in a directory named "split-decls-rs", it will be incorrectly skipped
Extract the package name from minimal_cargo_toml.package instead of the directory name for accurate self-crate detection.
🤖 Prompt for AI Agents
In src/main.rs around lines 90 to 99, the current self-crate check compares the
directory name to the literal "split-decls-rs", which is unreliable; instead
read the package name from the parsed Cargo.toml (e.g.,
minimal_cargo_toml.package.name) and compare that package name to the current
tool's package name; if the manifest package name is unavailable, fall back to
the directory-derived crate_name for safety, and then skip when the manifest
name equals the tool's package name (and update the printed message to include
which name was used).
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (2)
beginners_readme.md (2)
107-118: Add language identifier to fenced code block.The directory structure code block at line 107 lacks a language specification. Add
textto satisfy markdown linting rules.
169-187: Add language identifiers to directory structure code blocks.Both "Before" (line 169) and "After" (line 177) directory structure blocks lack language specifications. Add
textas the language identifier.
🧹 Nitpick comments (3)
my_test_project/Cargo.toml (1)
6-12: Consider removing empty dependency sections.Empty
[dependencies],[dev-dependencies],[build-dependencies], and[patch]sections can be omitted until needed. This reduces file size and improves readability.🔎 Suggested simplification
[package] name = "my_test_project" version = "0.1.0" edition = "2021" - -[dependencies] - -[dev-dependencies] - -[build-dependencies] - -[patch]my_test_project/build.new (1)
1-1: Large amount of unused imports and dead code.The following are imported but never used:
Span,TokenStream,quote,LitStr,fs,Path,HashMap,Item,visit,VisitMut,tomlThe macros
GetTokenandmkImplCallVisitorare defined but never invoked.Consider removing unused code to improve maintainability, or clarify if this is scaffolding for future functionality.
my_test_project/build.rs (1)
1-1: Unused imports and dead macro definitions.Same as
build.new: multiple unused imports (Span,TokenStream,quote,LitStr,fs,Path,HashMap,Item,visit,VisitMut,toml) and unused macro definitions (GetToken,mkImplCallVisitor).
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
beginners_readme.mdmy_test_project/.split-decls-config.tomlmy_test_project/Cargo.newmy_test_project/Cargo.tomlmy_test_project/build.newmy_test_project/build.rsmy_test_project/oldCargo.tomlmy_test_project/oldbuild.rsmy_test_project/src/decls/_decl_module_invocation.rsmy_test_project/src/decls/my_test_project_decls_TestStruct.rsmy_test_project/src/decls/my_test_project_decls_add.rsmy_test_project/src/decls/my_test_project_decls_hello.rsmy_test_project/src/lib.newmy_test_project/src/lib.rsmy_test_project/src/oldlib.rssplit-decls-rs.toml
✅ Files skipped from review due to trivial changes (1)
- my_test_project/Cargo.new
🧰 Additional context used
🧬 Code graph analysis (3)
my_test_project/src/decls/my_test_project_decls_hello.rs (1)
my_test_project/src/oldlib.rs (1)
hello(1-3)
my_test_project/src/oldlib.rs (2)
my_test_project/src/decls/my_test_project_decls_hello.rs (1)
hello(1-1)my_test_project/src/decls/my_test_project_decls_add.rs (1)
add(1-1)
my_test_project/src/decls/my_test_project_decls_add.rs (1)
my_test_project/src/oldlib.rs (1)
add(5-7)
🪛 LanguageTool
beginners_readme.md
[grammar] ~160-~160: Use a hyphen to join words.
Context: ...lies string replacements** to the backed up content 5. Splits code into indiv...
(QB_NEW_EN_HYPHEN)
🪛 markdownlint-cli2 (0.18.1)
beginners_readme.md
107-107: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
169-169: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
177-177: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (10)
my_test_project/Cargo.toml (1)
1-12: LGTM! Package manifest correctly configured.The manifest correctly specifies edition "2021" as per the PR objectives. The empty dependency sections are syntactically valid.
my_test_project/.split-decls-config.toml (1)
1-9: LGTM! Configuration correctly structured.The string replacement configuration (
"World"→"Universe") aligns with the updatedhello()function output and is consistent with the broader configuration structure used insplit-decls-rs.tomlandtest_config.toml.my_test_project/src/decls/_decl_module_invocation.rs (1)
1-1: LGTM! Module invocation correctly references declaration files.The
decl_module!macro correctly references the three declaration files added in this PR (my_test_project_decls_hello,my_test_project_decls_add, andmy_test_project_decls_TestStruct).split-decls-rs.toml (1)
1-4: LGTM! Configuration simplified appropriately.The streamlined configuration with the
"World"→"Universe"string replacement is consistent with the per-crate configurations (.split-decls-config.toml) and the updated function outputs. Removing the extensive commented sections improves maintainability.my_test_project/src/decls/my_test_project_decls_hello.rs (1)
1-1: The naming conflict scenario described does not exist. While the code correctly identifies thatdeclsmodule is publicly re-exported viapub use decls::*;in lib.rs, the verification shows thatoldlibis not publicly re-exported. There is nopub use oldlib::*;statement in lib.rs, so the ambiguous symbol conflict from having both modules exported cannot occur. Thehello()functions inoldlib.rsand the decls submodule do not create a naming conflict at the crate root level.Likely an incorrect or invalid review comment.
my_test_project/src/decls/my_test_project_decls_TestStruct.rs (1)
1-1: No naming conflict exists. Theoldlibmodule is not re-exported at the crate root—onlydeclsis re-exported viapub use decls::*in lib.rs. Although bothmy_test_project_decls_TestStruct.rsandoldlib.rsdefine identicalTestStructstructs, they do not collide becauseoldlibis not part of the crate's public module tree.Likely an incorrect or invalid review comment.
my_test_project/src/decls/my_test_project_decls_add.rs (1)
1-1: Generated declaration file looks correct.This is auto-generated code from the split-decls workflow. The
prelude!{}macro and#[decl_...]tag follow the expected pattern. The function implementation matches the original inoldlib.rs(which serves as the backup).my_test_project/src/lib.rs (1)
1-1: Generated gateway file follows the expected structure.The file correctly re-exports the prelude macros and exposes the
declsmodule at the crate root. This aligns with the split-decls workflow described in the documentation.beginners_readme.md (1)
264-282: Known limitations are clearly documented.The documentation transparently acknowledges current limitations (patch functionality needs testing, only simple projects tested, dependencies may need additions). The corrected configuration format section with a note about patches needing investigation is helpful for users.
my_test_project/src/oldlib.rs (1)
1-11: Original source correctly backed up.This file serves as the backup of the original
lib.rscontent before split-decls processing. The difference between"Hello, World!"here and"Hello, Universe!"inmy_test_project_decls_hello.rsdemonstrates the string replacement feature working as documented.
| 1. **Tool starts** and loads `split-decls-rs.toml` | ||
| 2. **Finds Rust projects** in the specified directory | ||
| 3. **Backs up original** `lib.rs` → `oldlib.rs` | ||
| 4. **Applies string replacements** to the backed up content |
There was a problem hiding this comment.
Minor grammar: use hyphenated compound adjective.
"backed up content" should be "backed-up content" when used as a compound adjective.
🔎 Proposed fix
-4. **Applies string replacements** to the backed up content
+4. **Applies string replacements** to the backed-up content🧰 Tools
🪛 LanguageTool
[grammar] ~160-~160: Use a hyphen to join words.
Context: ...lies string replacements** to the backed up content 5. Splits code into indiv...
(QB_NEW_EN_HYPHEN)
🤖 Prompt for AI Agents
In beginners_readme.md around line 160, the phrase "backed up content" is used
as a compound adjective and should be hyphenated; edit the line to replace
"backed up content" with "backed-up content" (and scan nearby sentences for the
same usage to apply the hyphen consistently).
| @@ -0,0 +1 @@ | |||
| use anyhow :: Context ; use anyhow :: Result ; use proc_macro2 :: { Span , TokenStream } ; use quote :: quote ; use syn :: LitStr ; use std :: fs ; use std :: path :: { Path , PathBuf } ; use std :: collections :: HashMap ; use syn :: { self , Item } ; use syn :: visit :: { self , Visit } ; use syn :: visit_mut :: { self , VisitMut } ; use split_decls_types :: { SplitDeclsConfig , PatchSpec , StringReplacement } ; use toml ; macro_rules ! GetToken { ($ token : tt) => { syn :: token :: $ token :: new (proc_macro2 :: Span :: call_site ()) } ; } macro_rules ! mkImplCallVisitor { (calls : $ init_calls : expr) => { struct ImplCallVisitor { calls : std :: collections :: HashMap < String , std :: collections :: HashSet < String >> , } impl ImplCallVisitor { fn new () -> Self { ImplCallVisitor { calls : $ init_calls } } } impl < 'ast > syn :: visit :: Visit < 'ast > for ImplCallVisitor { fn visit_macro (& mut self , i : & 'ast syn :: Macro) { if let Some (path_segment) = i . path . segments . last () { let path_str = path_segment . ident . to_string () ; if path_str . ends_with ("_impl") { if let Some (module_ident) = i . path . segments . first () { let module_name = module_ident . ident . to_string () ; let fn_name = path_str ; self . calls . entry (module_name) . or_default () . insert (fn_name) ; } } } syn :: visit :: visit_macro (self , i) ; } } } ; } fn main () -> Result < () > { println ! ("cargo:rerun-if-changed=build.rs") ; println ! ("cargo:rerun-if-changed{}" , "my_test_project\\src\\oldlib.rs") ; println ! ("cargo:rerun-if-changed{}" , "my_test_project\\oldbuild.rs") ; println ! ("cargo:rerun-if-changed=.split-decls-config.toml") ; let config_path = PathBuf :: from (std :: env :: var ("CARGO_MANIFEST_DIR") ?) . join (".split-decls-config.toml") ; let config = SplitDeclsConfig :: load_from_file (& config_path) . context (format ! ("Failed to load config from {}" , config_path . display ())) ? ; let current_crate_name_for_patch = "my_test_project" . to_string () ; if let Some (patches_for_crate) = config . patches . get (& current_crate_name_for_patch) { for patch_spec in patches_for_crate { let patch_path = PathBuf :: from (std :: env :: var ("CARGO_MANIFEST_DIR") ?) . join (& patch_spec . path) ; println ! ("cargo:rerun-if-changed={}" , patch_path . display ()) ; } } println ! ("cargo:note=build.rs finished. If `split-decls-rs` needs to be re-run, changes will be detected.") ; Ok (()) } No newline at end of file | |||
There was a problem hiding this comment.
Malformed cargo:rerun-if-changed directives will break build caching.
The directives are missing the = sign between the key and value:
println ! ("cargo:rerun-if-changed{}" , "my_test_project\\src\\oldlib.rs")
Should be:
println!("cargo:rerun-if-changed={}", ...)
Additionally, the paths use Windows-style separators (\\) and are hardcoded rather than computed relative to CARGO_MANIFEST_DIR, breaking cross-platform compatibility.
🔎 Proposed fix pattern
-println ! ("cargo:rerun-if-changed{}" , "my_test_project\\src\\oldlib.rs") ;
-println ! ("cargo:rerun-if-changed{}" , "my_test_project\\oldbuild.rs") ;
+let manifest_dir = PathBuf::from(std::env::var("CARGO_MANIFEST_DIR")?);
+println!("cargo:rerun-if-changed={}", manifest_dir.join("src/oldlib.rs").display());
+println!("cargo:rerun-if-changed={}", manifest_dir.join("oldbuild.rs").display());Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In my_test_project/build.new around lines 1-1, the cargo directives are
malformed and platform-unsafe: the println! calls use "cargo:rerun-if-changed{}"
(missing the "=") and hardcoded Windows backslashes; fix by formatting the
directive as println!("cargo:rerun-if-changed={}", path) and build the path
using PathBuf::from(std::env::var("CARGO_MANIFEST_DIR")?).join("relative/path")
(or join(&patch_spec.path)), then pass path.display() (or to_string_lossy()) to
println! so paths are computed relative to CARGO_MANIFEST_DIR and
platform-correct.
| @@ -0,0 +1 @@ | |||
| use anyhow :: Context ; use anyhow :: Result ; use proc_macro2 :: { Span , TokenStream } ; use quote :: quote ; use syn :: LitStr ; use std :: fs ; use std :: path :: { Path , PathBuf } ; use std :: collections :: HashMap ; use syn :: { self , Item } ; use syn :: visit :: { self , Visit } ; use syn :: visit_mut :: { self , VisitMut } ; use split_decls_types :: { SplitDeclsConfig , PatchSpec , StringReplacement } ; use toml ; macro_rules ! GetToken { ($ token : tt) => { syn :: token :: $ token :: new (proc_macro2 :: Span :: call_site ()) } ; } macro_rules ! mkImplCallVisitor { (calls : $ init_calls : expr) => { struct ImplCallVisitor { calls : std :: collections :: HashMap < String , std :: collections :: HashSet < String >> , } impl ImplCallVisitor { fn new () -> Self { ImplCallVisitor { calls : $ init_calls } } } impl < 'ast > syn :: visit :: Visit < 'ast > for ImplCallVisitor { fn visit_macro (& mut self , i : & 'ast syn :: Macro) { if let Some (path_segment) = i . path . segments . last () { let path_str = path_segment . ident . to_string () ; if path_str . ends_with ("_impl") { if let Some (module_ident) = i . path . segments . first () { let module_name = module_ident . ident . to_string () ; let fn_name = path_str ; self . calls . entry (module_name) . or_default () . insert (fn_name) ; } } } syn :: visit :: visit_macro (self , i) ; } } } ; } fn main () -> Result < () > { println ! ("cargo:rerun-if-changed=build.rs") ; println ! ("cargo:rerun-if-changed{}" , "my_test_project\\src\\oldlib.rs") ; println ! ("cargo:rerun-if-changed{}" , "my_test_project\\oldbuild.rs") ; println ! ("cargo:rerun-if-changed=.split-decls-config.toml") ; let config_path = PathBuf :: from (std :: env :: var ("CARGO_MANIFEST_DIR") ?) . join (".split-decls-config.toml") ; let config = SplitDeclsConfig :: load_from_file (& config_path) . context (format ! ("Failed to load config from {}" , config_path . display ())) ? ; let current_crate_name_for_patch = "my_test_project" . to_string () ; if let Some (patches_for_crate) = config . patches . get (& current_crate_name_for_patch) { for patch_spec in patches_for_crate { let patch_path = PathBuf :: from (std :: env :: var ("CARGO_MANIFEST_DIR") ?) . join (& patch_spec . path) ; println ! ("cargo:rerun-if-changed={}" , patch_path . display ()) ; } } println ! ("cargo:note=build.rs finished. If `split-decls-rs` needs to be re-run, changes will be detected.") ; Ok (()) } No newline at end of file | |||
There was a problem hiding this comment.
Malformed cargo:rerun-if-changed directives will break build caching.
Same issue as build.new - the directives are missing the = sign:
println ! ("cargo:rerun-if-changed{}" , "my_test_project\\src\\oldlib.rs")
This will not register the files for rebuild detection. The paths are also hardcoded with Windows separators.
🔎 Proposed fix pattern
-println ! ("cargo:rerun-if-changed{}" , "my_test_project\\src\\oldlib.rs") ;
-println ! ("cargo:rerun-if-changed{}" , "my_test_project\\oldbuild.rs") ;
+let manifest_dir = PathBuf::from(std::env::var("CARGO_MANIFEST_DIR")?);
+println!("cargo:rerun-if-changed={}", manifest_dir.join("src/oldlib.rs").display());
+println!("cargo:rerun-if-changed={}", manifest_dir.join("oldbuild.rs").display());🤖 Prompt for AI Agents
In my_test_project/build.rs around lines 1 to 1, the two println! calls use the
wrong format string ("cargo:rerun-if-changed{}") and hardcoded Windows
backslashes; change them to include the missing '=' (e.g.
"cargo:rerun-if-changed={}") and pass a Path/PathBuf's display() or the
already-built patch_path so the platform-correct path is printed (avoid
hardcoded backslash strings). Ensure both occurrences are updated similarly so
cargo sees the correct directives.
| [package] | ||
| name = "my_test_project" | ||
| version = "0.1.0" | ||
| edition = "2021" No newline at end of file |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if both oldCargo.toml and Cargo.toml exist in my_test_project
fd -t f "Cargo.toml|oldCargo.toml" my_test_project/ --max-depth 1Repository: meta-introspector/split-decls-rs
Length of output: 132
🏁 Script executed:
#!/bin/bash
# Check contents of both files and look for documentation
echo "=== my_test_project/Cargo.toml ==="
cat -n my_test_project/Cargo.toml
echo -e "\n=== my_test_project/oldCargo.toml ==="
cat -n my_test_project/oldCargo.toml
echo -e "\n=== Files in my_test_project directory ==="
ls -la my_test_project/ | head -20Repository: meta-introspector/split-decls-rs
Length of output: 1120
🏁 Script executed:
#!/bin/bash
# Check if there's documentation in the repo about these files
echo "=== Checking for README or documentation ==="
find . -maxdepth 2 -name "README*" -o -name "*.md" | head -10
echo -e "\n=== Checking root for any docs about split-decls ==="
head -20 README.md 2>/dev/null || echo "No README.md at root"Repository: meta-introspector/split-decls-rs
Length of output: 2757
Clarify the purpose of oldCargo.toml alongside Cargo.toml.
Both files exist in the same directory. oldCargo.toml appears to contain the original package configuration before transformation, while Cargo.toml contains the full manifest structure. If these are intentional test fixtures demonstrating the tool's transformation behavior, document this in a comment or README. If they are accidental artifacts, consider removing them.
🤖 Prompt for AI Agents
In my_test_project/oldCargo.toml around lines 1 to 4, there's an ambiguous
duplicate of Cargo.toml (appears to be an original/fixture copy) that isn't
documented; either document its purpose (e.g., add a top-line comment explaining
it's a test fixture/original manifest used to demonstrate transformation and
reference it in the repo README), or remove the file if it is an unintended
artifact; update README or add a short comment at the top of oldCargo.toml
stating it is an intentional fixture and why, and ensure CI/packaging ignores it
if necessary.
| @@ -0,0 +1 @@ | |||
| pub use introspector_decl2_macros :: prelude :: * ; pub mod decls ; pub use decls :: * ; No newline at end of file | |||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for lib.rs and lib.new to understand the structure
echo "=== Checking for lib files in my_test_project ==="
fd -t f "lib\.(rs|new)" my_test_project/src
echo -e "\n=== Comparing content if both exist ==="
if [ -f my_test_project/src/lib.rs ] && [ -f my_test_project/src/lib.new ]; then
echo "lib.rs content:"
cat my_test_project/src/lib.rs
echo -e "\nlib.new content:"
cat my_test_project/src/lib.new
fi
echo -e "\n=== Check Cargo.toml to understand project type ==="
if [ -f my_test_project/Cargo.toml ]; then
cat my_test_project/Cargo.toml
fiRepository: meta-introspector/split-decls-rs
Length of output: 651
Remove or clarify the purpose of lib.new—it's redundant with lib.rs.
Both lib.rs and lib.new contain identical code and Cargo will only compile lib.rs (the standard library crate root). Delete lib.new unless it serves a specific purpose in your migration process or build workflow. If it's temporary, add it to .gitignore to prevent confusion.
🤖 Prompt for AI Agents
In my_test_project/src/lib.new around lines 1 to 1, lib.new duplicates the crate
root contents already in lib.rs and is redundant because Cargo only uses lib.rs;
either delete src/lib.new to remove confusion or, if it's intentionally kept as
a temporary/migration file, add src/lib.new to .gitignore and add a short
comment at the top explaining its purpose and temporary status so maintainers
know why it exists.
1 participant
PR Type
Enhancement, Bug fix
Description
Migrate from Rust 2024 to 2021 edition across all crates
Replace workspace dependencies with explicit version specifications
Remove invalid proc-macro2 patch and pagerank_rs dependency
Add test configuration file for split-decls-rs
Simplify Scalar.rs macro implementation to async_graphql scalar macro
Diagram Walkthrough
File Walkthrough
Scalar.rs
Simplify Scalar macro to async_graphqloutput/macros/Scalar.rs
async_graphql scalar! macro
async_graphql functionality
Cargo.toml
Remove workspace, fix edition, explicit versionsCargo.toml
specifications
Cargo.toml
Update edition and dependency referencesbuild_helpers/cargo_toml_parts/Cargo.toml
Cargo.toml
Update proc-macro2 to explicit versioncargo-toml-generator-macros/Cargo.toml
"1.0"
Cargo.toml
Update proc-macro2 to explicit versioncargo-toml-generator-types/Cargo.toml
"1.0"
Cargo.toml
Update edition and dependency versionssplit-decls-types/Cargo.toml
specifications
test_config.toml
Add test configuration filetest_config.toml
string_replacements sections
Summary by CodeRabbit
New Features
Documentation
Chores
Tests
✏️ Tip: You can customize this high-level summary in your review settings.