fix(table): Fix header separator logic and add tests for HTML table conversion - #463
Merged
Merged
Conversation
tanahiro2010
changed the title
tanahiro2010
changed the title
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes Markdown header-separator emission in the table conversion utilities when leading empty rows/lines are skipped, ensuring the separator is inserted after the first emitted row.
Changes:
- Update
htmlTableToMarkdownto insert the header separator after the first non-empty<tr>that is actually output. - Update
convertTsvCsvToMarkdownto insert the header separator after the first non-empty line that is actually output. - Add an HTML regression test covering a leading empty
<tr>.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/utils/tableConverter.ts |
Fixes header-separator insertion to key off the first emitted row, not the raw loop index, for both HTML and TSV/CSV conversion paths. |
src/utils/__tests__/tableConverter.test.ts |
Adds a regression test for HTML tables with a leading empty row (and includes commentary updates needing cleanup). |
Suppressed comments (1)
src/utils/tests/tableConverter.test.ts:156
- This inline note says the assertion "currently fails", but with the header-separator fix applied this should now pass. Leaving it in will confuse future readers/debugging.
expect(lines[1]).toContain('---'); // currently fails: lines[1] is '| 1 | 2 |'
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Remove references to old loop indices (i === 0 / i === 1) from the T-TC-22 and line 156 comments, updating them to describe the regression context instead. Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
…version Pins the header-separator behavior for an empty leading line, the TSV/CSV counterpart to the empty-leading-row case fixed for HTML tables (T-TC-22), so a future change to trimming/loop logic can't silently drop the separator row again.
… be changed, so I made those corrections.
Collaborator
|
Thank you for your assistance. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This pull request fixes a bug in the table-to-Markdown conversion logic where an empty leading row in an HTML table or CSV/TSV input would cause the Markdown header separator row to be omitted. The fix ensures that the separator is always inserted after the first non-empty row, regardless of any skipped empty rows. A new test case is also added to verify this behavior.
Bug fix for header separator logic:
src/utils/tableConverter.ts: The logic for inserting the Markdown header separator row now checks if the current row is the first emitted (non-empty) row, rather than relying on the loop index. This prevents empty leading rows from breaking the output. [1] [2]Test coverage:
src/utils/__tests__/tableConverter.test.ts: Adds a test (T-TC-22) to confirm that an empty leading row does not prevent the header separator from being emitted in the Markdown output.