-
-
Notifications
You must be signed in to change notification settings - Fork 6k
Disable Field count validation of CSV viewer #35228
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
Conversation
Default behaviour rejected all rows (Records) with more or fewer columns (Fields) than the first row, preventing them from rendering at all and silently hiding them.
Unit tests need to be updated/extended:
--- FAIL: TestDetermineDelimiterReadAllError (0.00s)
csv_test.go:132:
Error Trace: /home/runner/work/gitea/gitea/modules/csv/csv_test.go:132
Error: An error is expected but got nil.
Test: TestDetermineDelimiterReadAllError
Messages: RaadAll() should throw error
csv_test.go:133:
Error Trace: /home/runner/work/gitea/gitea/modules/csv/csv_test.go:133
Error: Target error should be in err chain:
expected: "wrong number of fields"
in chain:
Test: TestDetermineDelimiterReadAllError
csv_test.go:134:
Error Trace: /home/runner/work/gitea/gitea/modules/csv/csv_test.go:134
Error: Should be empty, but was [[col1 col2] [a;b] [c@e] [f g] [h|i] [jkl]]
Test: TestDetermineDelimiterReadAllError
Messages: rows should be empty
I see that test case (TestDetermineDelimiterReadAllError) is roughly the same input as TestGuessDelimiter case 16. If there's nothing else it's testing for, perhaps it should be changed to a third case for TestCreateReaderAndDetermineDelimiter?
What will happen to CSV file comparisons after this change?
What will happen to CSV file comparisons after this change?
The missing columns seem to already be empty cells, perhaps because the comparison code already needed to handle changing number of columns.
image
image
Personally I find the side-by-side rendered diff to be too confusing to read and am thinking about doing row-by-row comparisons.
Yeah, row-based diff would be similar to git diff and likely more readable. Probably better to split that out into another PR.
need one more review
* giteaofficial/main: Refactor and update mail templates (go-gitea#35150) Disable Field count validation of CSV viewer (go-gitea#35228) split admin config settings templates to make it maintain easier (go-gitea#35294) Update tools/package.json dependencies, remove imagemin-zopfli (go-gitea#35406)
Default behaviour rejected all rows (Records) with more or fewer columns (Fields) than the first row, preventing them from parsing at all and silently hiding them. While RFC4180 section 2.4 says each line should contain the same number of fields, enforcing this on the viewer is unhelpful.
This pull request disables that validation, allowing the viewer to render lines with fewer columns than the maximum number within the file. As it's a simple HTML table, this works without additional changes (i.e. no need to manually determine the maximum number of columns), but the default appearance of rows with fewer columns may be undesirable to some people, especially when using CSS that has
td {border-right: none}.Screenshot without cell right borders
Screenshot with cell right borders
Fixes #16559, #30358.
Unfortunately, retaining empty lines is less trivial, so the line numbers on the leftmost column will still not match the source file whenever those are present, though a future PR could address that.