feat: --no-file-summary now also suppresses generationHeader; doesn't suppress headerText (fixes #554) #556
Conversation
… suppress headerText (fixes yamadashy#554)
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hello @joshwand, 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!
Summary of Changes
Hello team, gemini-code-assist here with a summary of this pull request. This PR implements a feature requested in the title: modifying the behavior of the --no-file-summary flag. Previously, this flag would suppress the file summary section but still include the automatically generated generationHeader and the user-provided headerText. With this change, --no-file-summary (or fileSummaryEnabled: false in the configuration) will now correctly suppress the generationHeader as well, while ensuring that the user-provided headerText is always included if specified, regardless of whether the file summary is enabled. This change affects the output generation logic and the templates for Markdown, Plain, and XML styles. New tests have been added to verify this updated behavior across the different output formats.
Highlights
- Feature Implementation: Modifies the behavior of the
--no-file-summaryflag to align with the requested functionality. - Conditional Output: The automatically generated
generationHeaderis now only included in the output when the file summary is enabled (fileSummaryEnabled: true). - Unconditional Output: The user-provided
headerTextis now always included in the output if it is specified, regardless of thefileSummaryEnabledsetting. - Template Updates: The Handlebars templates for Markdown, Plain, and XML output styles have been updated to reflect the new conditional logic for the
generationHeaderandheaderText. - New Tests: Adds comprehensive tests to
outputGenerate.test.tsand the style-specific test files (markdownStyle.test.ts,plainStyle.test.ts,xmlStyle.test.ts) to ensure the correct header behavior when the file summary is disabled.
Changelog
Click here to see the changelog
- src/core/output/outputGenerate.ts
- Adjusted the XML output structure (
xmlDocument) to place thegenerationHeaderinside thefile_summaryelement (making it conditional onfileSummaryEnabled) and move theuser_provided_headeroutside thefile_summaryelement (making it unconditional).
- Adjusted the XML output structure (
- src/core/output/outputStyles/markdownStyle.ts
- Moved the
{{{generationHeader}}}helper call inside the{{#if fileSummaryEnabled}}block. - Moved the
{{#if headerText}}...{{/if}}block outside the{{#if fileSummaryEnabled}}...{{/if}}block. - Added necessary imports for testing utilities.
- Moved the
- src/core/output/outputStyles/plainStyle.ts
- Moved the
{{{generationHeader}}}helper call inside the{{#if fileSummaryEnabled}}block. - Moved the
{{#if headerText}}...{{/if}}block outside the{{#if fileSummaryEnabled}}...{{/if}}block.
- Moved the
- src/core/output/outputStyles/xmlStyle.ts
- Moved the
{{{generationHeader}}}helper call inside the{{#if fileSummaryEnabled}}block. - Moved the
{{#if headerText}}...{{/if}}block outside the{{#if fileSummaryEnabled}}...{{/if}}block. - Adjusted XML element nesting to accommodate the moved
headerText.
- Moved the
- tests/core/output/outputGenerate.test.ts
- Added new tests to
generateOutputfor plain, XML, and markdown styles to verify that thegenerationHeaderis omitted andheaderTextis included whenfileSummaryEnabledis false.
- Added new tests to
- tests/core/output/outputStyles/markdownStyle.test.ts
- Added template-specific tests for the markdown style to confirm that
headerTextis displayed andgenerationHeaderis not displayed whenfileSummaryEnabledis false. - Added imports for
generateOutputandcreateMockConfig.
- Added template-specific tests for the markdown style to confirm that
- tests/core/output/outputStyles/plainStyle.test.ts
- Added a template-specific test for the plain style to confirm the header behavior when
fileSummaryEnabledis false.
- Added a template-specific test for the plain style to confirm the header behavior when
- tests/core/output/outputStyles/xmlStyle.test.ts
- Added a template-specific test for the xml style to confirm the header behavior when
fileSummaryEnabledis false.
- Added a template-specific test for the xml style to confirm the header behavior when
Using Gemini Code Assist
The 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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and 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 to provide feedback.
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.
A flag flips its state,
Header logic shifts its fate,
User text stays true.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
joshwand
changed the title
There was a problem hiding this comment.
Code Review
This pull request effectively addresses issue #554 by ensuring that --no-file-summary (or fileSummaryEnabled: false) suppresses the generationHeader across all output formats, while correctly preserving headerText if provided. The changes are well-localized to the output generation and templating logic.
The addition of new test cases for various output styles (plain, XML, Markdown) and for both parsable and template-based XML is commendable, ensuring good coverage for the modified behavior. The tests clearly verify the suppression of generationHeader and the persistence of headerText under the specified conditions.
One area for clarification is the structural change in the parsable XML output when fileSummaryEnabled is true, which I've detailed in a specific comment.
Summary of Findings
- Parsable XML Structure Change: When
fileSummaryEnabledis true, thegenerationHeaderin parsable XML output has moved from being the text content of the root<repomix>element to being the text content of the<file_summary>element. This also overwrites the previous default text of<file_summary>. This is a structural change that might affect existing consumers of the parsable XML format and warrants confirmation. - Test Coverage: The PR includes good test coverage for the new behavior, especially for the suppression of
generationHeaderwhenfileSummaryEnabledis false. - Consistency Across Formats: The change makes the placement of
generationHeader(as part of the summary section, if enabled) consistent across different output formats (Markdown, plain, template XML, and parsable XML).
Merge Readiness
The pull request correctly implements the primary goal of suppressing generationHeader when --no-file-summary is used. The added tests are valuable.
However, there's a structural change to the parsable XML output when the file summary is enabled, which could be a breaking change for existing consumers. I've raised a medium severity comment regarding this. It would be beneficial to clarify the intent behind this specific structural change before merging.
I am unable to approve pull requests myself, but I recommend addressing or acknowledging the point raised about the parsable XML structure to ensure clarity and avoid unintended consequences for users of that format. Once that's clarified, the PR should be in good shape.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #556 +/- ##
==========================================
+ Coverage 88.39% 88.41% +0.01%
==========================================
Files 91 91
Lines 4679 4678 -1
Branches 935 937 +2
==========================================
Hits 4136 4136
+ Misses 543 542 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull Request Overview
This PR updates output generation templates and tests to ensure that the generationHeader is suppressed when fileSummary is disabled while still displaying the headerText regardless of fileSummary configuration.
- Added tests across xml, plain, and markdown styles for the new behavior
- Updated templates in XML, plain, and markdown output generators to conditionally render generationHeader inside fileSummary blocks and always render headerText
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| tests/core/output/outputStyles/xmlStyle.test.ts | Added new test to check xml style behavior with fileSummary disabled |
| tests/core/output/outputStyles/plainStyle.test.ts | Added new test to verify plain style displays headerText while suppressing generationHeader |
| tests/core/output/outputStyles/markdownStyle.test.ts | Added new tests for markdown style ensuring proper headerText and suppression of generationHeader |
| tests/core/output/outputGenerate.test.ts | Added new tests for output generation for txt, xml, and markdown styles |
| src/core/output/outputStyles/xmlStyle.ts | Updated XML template to conditionally render generationHeader and always render headerText |
| src/core/output/outputStyles/plainStyle.ts | Updated plain template to conditionally render generationHeader and consistently display headerText |
| src/core/output/outputStyles/markdownStyle.ts | Updated markdown template to conditionally render generationHeader and move headerText block |
| src/core/output/outputGenerate.ts | Adjusted XML output generation to remove extra header wrapping and ensure headerText is top-level |
…dedHeader, Directory Structure) across all output styles
There was a problem hiding this comment.
Pull Request Overview
This PR ensures that disabling file summaries (--no-file-summary) suppresses only the automatically generated header, while still showing any user-specified headerText.
- Added and updated integration and unit tests for XML, plain, and Markdown styles to cover the new behavior.
- Modified Handlebars templates (
xmlStyle,plainStyle,markdownStyle) and the XML builder (generateParsableXmlOutput) to conditionally wrap generation headers and always includeheaderText.
Reviewed Changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/integration-tests/packager.test.ts | Updated config shape, added Markdown style case, and clarified assertions. |
| tests/integration-tests/fixtures/packager/outputs/simple-project-output.xml | Added a "Repository files" entry and relocated closing tags for user-provided header. |
| tests/integration-tests/fixtures/packager/outputs/simple-project-output.txt | Inserted user-provided header section into plain-text fixture. |
| tests/integration-tests/fixtures/packager/outputs/simple-project-output.md | Added the expected Markdown output fixture. |
| tests/core/output/outputStyles/xmlStyle.test.ts | New test verifying XML header behavior when file summaries are disabled. |
| tests/core/output/outputStyles/plainStyle.test.ts | New test verifying plain text header behavior when file summaries are disabled. |
| tests/core/output/outputStyles/markdownStyle.test.ts | Updated expectations and added tests for Markdown header/generation-header logic. |
| tests/core/output/outputGenerate.test.ts | Added integration tests for generateOutput across all three styles and flags. |
| src/core/output/outputStyles/xmlStyle.ts | Wrapped generation header & file summary in if fileSummaryEnabled; moved headerText. |
| src/core/output/outputStyles/plainStyle.ts | Wrapped generation header & summary in if fileSummaryEnabled; moved headerText. |
| src/core/output/outputStyles/markdownStyle.ts | Wrapped generation header & summary in if fileSummaryEnabled; moved headerText. |
| src/core/output/outputGenerate.ts | Moved generationHeader into file_summary and always include user_provided_header. |
tests/integration-tests/fixtures/packager/outputs/simple-project-output.xml
Outdated
Show resolved
Hide resolved
tests/integration-tests/fixtures/packager/outputs/simple-project-output.md
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull Request Overview
This PR ensures that the --no-file-summary option suppresses the auto-generated header but still includes any user-provided header text.
- Wrapped
generationHeaderinfileSummaryEnabledguards and moved theheaderTextblock outside that guard in all output templates (XML, plain, markdown). - Added new unit tests for each style to verify that
headerTextalways appears andgenerationHeaderis conditional, plus updated integration tests and fixtures for markdown and plain outputs.
Reviewed Changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| tests/integration-tests/packager.test.ts | Expanded the integration matrix to cover markdown style and updated assertions for headers. |
| tests/integration-tests/fixtures/packager/outputs/*.xml | Adjusted numbering and removed <additional_info> wrapper around <user_provided_header>. |
| tests/integration-tests/fixtures/packager/outputs/*.txt | Updated separators and block for “User Provided Header” to match plain template changes. |
| tests/integration-tests/fixtures/packager/outputs/*.md | Added a full markdown output fixture including user header and code blocks. |
| tests/core/output/outputStyles/xmlStyle.test.ts | Added a test for XML style to check headerText vs. generationHeader behavior. |
| tests/core/output/outputStyles/plainStyle.test.ts | Added a similar test for plain style. |
| tests/core/output/outputStyles/markdownStyle.test.ts | Updated expectations to match the new single-# header for user-provided text. |
| tests/core/output/outputGenerate.test.ts | New tests to verify generateOutput behavior across all three styles. |
| src/core/output/outputStyles/xmlStyle.ts | Wrapped generationHeader in fileSummaryEnabled and moved headerText out. |
| src/core/output/outputStyles/plainStyle.ts | Same wrap/move logic applied for plain text template. |
| src/core/output/outputStyles/markdownStyle.ts | Applied the same pattern in the markdown template. |
| src/core/output/outputGenerate.ts | Updated XML builder to only set #text for file summary when enabled and always include headerText. |
Comments suppressed due to low confidence (2)
tests/core/output/outputStyles/xmlStyle.test.ts:35
- This test uses
createMockConfigandgenerateOutputbut neither is imported in this file. Please add:
import { generateOutput } from '../../../../src/core/output/outputGenerate.js';
import { createMockConfig } from '../../../testing/testUtils.js';const mockConfig = createMockConfig({
tests/core/output/outputStyles/plainStyle.test.ts:35
- This test uses
createMockConfigandgenerateOutputbut neither is imported in this file. Please add:
import { generateOutput } from '../../../../src/core/output/outputGenerate.js';
import { createMockConfig } from '../../../testing/testUtils.js';const mockConfig = createMockConfig({
|
@joshwand I made a few minor tweaks, and everything looks good! Please let me know if there’s anything else. |
--no-file-summary should also suppress generationHeader; should not suppress user-specified headerText ( fixes #554 )
Checklist
npm run testnpm run lint