Conversation
Drop 5 built-in .github/ template files (carson.md, AGENTS.md, CLAUDE.md, copilot-instructions.md, pull_request_template.md) that are now superseded by lint.canonical distribution. Default template.managed_files to empty. Add removed files to SUPERSEDED so template_apply! cleans them from governed repos. Fix stale-only propagation bug in refresh! where SUPERSEDED removals were not counted in drift totals, causing template_propagate! to skip when only stale files needed remote cleanup. Use fetch_optional_string_array for managed_files to allow empty defaults.
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request streamlines Carson's template management by removing all hardcoded, built-in Highlights
Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request effectively removes the built-in template files, shifting the responsibility to user-provided canonical files via lint.canonical. The changes are consistent across documentation, application logic, and tests. The old template files are now correctly marked as superseded to ensure they are cleaned up from existing installations. I have one suggestion to improve maintainability in the test suite by ensuring test logic directly reflects application behavior, thereby preventing unintended divergence.
| def pre_populate_templates!( repo_root:, tool_root:, config_path: nil ) | ||
| env = config_path ? { "CARSON_CONFIG_FILE" => config_path } : {} | ||
| cfg = with_env( env ) { Carson::Config.load( repo_root: repo_root ) } | ||
| cfg.template_managed_files.each do |managed_file| | ||
| relative_within_github = managed_file.delete_prefix( ".github/" ) | ||
| template_path = File.join( templates_dir, relative_within_github ) | ||
| template_path = File.join( templates_dir, File.basename( managed_file ) ) unless File.file?( template_path ) | ||
| next unless File.file?( template_path ) | ||
| source = template_source_for( managed_file: managed_file, tool_root: tool_root, config: cfg ) | ||
| next unless source | ||
|
|
||
| target_path = File.join( repo_root, managed_file ) | ||
| FileUtils.mkdir_p( File.dirname( target_path ) ) | ||
| content = File.read( template_path ).gsub( "\r\n", "\n" ).rstrip + "\n" | ||
| content = File.read( source ).gsub( "\r\n", "\n" ).rstrip + "\n" | ||
| File.write( target_path, content ) | ||
| end | ||
| system( "git", "-C", repo_root, "add", "--all", out: File::NULL, err: File::NULL ) | ||
| system( "git", "-C", repo_root, "commit", "-m", "add templates", out: File::NULL, err: File::NULL ) | ||
| system( "git", "-C", repo_root, "push", "origin", "main", out: File::NULL, err: File::NULL ) | ||
| end |
There was a problem hiding this comment.
To improve maintainability and reduce code duplication, this test helper should be refactored. The template_source_for method (lines 304-320) duplicates the logic for resolving template source paths, which is already implemented in Carson::Runtime::Local#template_source_path. Relying on a separate implementation in the tests can lead to them becoming out of sync with the application code if the source path logic changes in the future.
I suggest modifying pre_populate_templates! to use the private template_source_path method from a Carson::Runtime instance via send. This will ensure your test setup always uses the same logic as the application. After applying this suggestion, the template_source_for helper method can be removed.
def pre_populate_templates!( repo_root:, tool_root:, config_path: nil )
env = config_path ? { "CARSON_CONFIG_FILE" => config_path } : {}
runtime = with_env( env ) do
# We need a runtime instance to access the real template_source_path logic.
Carson::Runtime.new( repo_root: repo_root, tool_root: tool_root, output: StringIO.new, error: StringIO.new )
end
runtime.config.template_managed_files.each do |managed_file|
source = runtime.send( :template_source_path, managed_file: managed_file )
next unless source && File.file?( source )
target_path = File.join( repo_root, managed_file )
FileUtils.mkdir_p( File.dirname( target_path ) )
content = File.read( source ).gsub( "\r\n", "\n" ).rstrip + "\n"
File.write( target_path, content )
end
system( "git", "-C", repo_root, "add", "--all", out: File::NULL, err: File::NULL )
system( "git", "-C", repo_root, "commit", "-m", "add templates", out: File::NULL, err: File::NULL )
system( "git", "-C", repo_root, "push", "origin", "main", out: File::NULL, err: File::NULL )
endReferences
- While the rule advises against premature abstraction if duplicated code is expected to diverge, in this specific scenario, the test's template source path resolution logic must accurately reflect the application's logic. Relying on the application's existing method directly ensures consistency and prevents unintended divergence, which would lead to incorrect test behavior. This approach aligns with avoiding incorrect abstractions by ensuring the test accurately models the system under test.
1 participant
No description provided.