Repository navigation
Trim comma-padded blank rows in CSV table conversion #2687
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
Chen Yufeiyang (feiiiiii5)
wants to merge
2
commits into
microsoft:main
Choose a base branch
from
feiiiiii5:fix/csv-comma-padded-blank-rows
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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.
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.
This also removes legitimate headers whose column names are empty. For example, converting
b",\nAlice,30\nBob,40\n"throughMarkItDown.convert_streampreviously produced:On this branch it produces:
Alice's record is now treated as the header. Quoted empty names have the same problem; a three-column unnamed header also loses its extra column.
Could we preserve field-bearing headers by default and make trimming leading padded records opt-in? Parsed empty fields alone can't distinguish padding from an unnamed header. I reproduced these three regressions on
c037894and checked that they pass on the base.Tested with Hermes AI assistance.
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.
Good catch, and reproduced: all three cases changed on this branch and matched
mainagain once I narrowed the rule. You're right that parsed empty fields alone can't separate padding from an unnamed header —pandas.read_csvreads a first row of,/,,intoUnnamed: 0/Unnamed: 1, so it resolves the ambiguity as a header. This branch now agrees: the all-empty-cells rule applies only to rows after the header (the run right after it, and the trailing run), and the leading run keeps the old "no fields at all" test._is_blank_rowis now_is_padding_row, with that caveat in its docstring. Done in 93cbcc6.I dropped the leading trim rather than gating it behind an opt-in, because nothing would set the flag and
pandas.to_csvputs its blank rows after the named header — the trailing and after-header cases are the ones that reproduce in practice. If you or a maintainer would still rather trim leading padding behind an option, say so and I'll add it.The three shapes you listed are now pinned as tests (
,,"","", and,,as the first row, including the column count).pytest tests/test_csv.py -q→ 47 passed; overtests/test_csv.py tests/test_module_misc.py tests/test_cu.py→ 325 passed here, against 322 passed plus the three trimming failures with only_csv_converter.pyput back at4cc9fa1.