CSV Renderer may guess a TSV file is comma delimited even when tab delimited

[richmahn]

• Gitea version (or commit ref): 4b6370c (main)
• Can you reproduce the bug at https://try.gitea.io:
  • Yes (provide example URL)
  • No

https://try.gitea.io/richmahn/test/src/branch/master/is_tsv_but_uses_commas.tsv

See the raw....it actually has 8 rows, yet because the "intro" row has many commas in its text, only rows that have NO commas (because the header has no commas), thus only one field, get shown.

Description

If a file is a .tsv file, which, while isn't a standard itself and thus uses CSV but with tab as the delimiter, but has more commas than tabs, the function here thinks the delimiter is comma, and thus all rows that have more fields than the header (which has no commas thus 1 field) get removed from the rendering.

I suggest, which I have done in my fork of Gitea, make a separate markup renderer like the "csv" one but call it "tsv" and always use \t as the delimiter due to the filename. Either that, or make sure the function that guesses the delimiter gets the filename of the file being rendered so it can determine from that.

Screenshots

Here is the blame for my .tsv file on try.gitea.io which shows it is tab delimited, with the 2nd row having LOTS of texts with commas:

Yet the table rendering removes many of the rows:

[lunny]

So I think that's a bug of the function guesses the delimiter.

[silverwind]

make a separate markup renderer like the "csv" one but call it "tsv" and always use \t as the delimiter due to the filename

I would not make a separate renderer but simply skip over guessDelimiter in case the extension provides a strong indication of the delimiter in use. That way, we can extend with more similar extensions later.

One such extension may be .psv for Pipe Separated Values (|).

[lunny]

Hm, We should allow external renderer has their own parameters in app.ini.

[richmahn]

Yes, we need to have the filename available so that it can be use in determining which delimiter to use as well.

[richmahn]

make a separate markup renderer like the "csv" one but call it "tsv" and always use \t as the delimiter due to the filename

I would not make a separate renderer but simply skip over guessDelimiter in case the extension provides a strong indication of the delimiter in use. That way, we can extend with more similar extensions later.

One such extension may be .psv for Pipe Separated Values (|).

Yes, but the problem is currently there is no extension passed to the renderer, as it is invoked by the list of extensions it renders. I offered that as a solution though, if we can get it passed in.

[richmahn]

Hm, We should allow external renderer has their own parameters in app.ini.

@lunny What kind of parameters? You mean what delimiter to use if it is a tsv, a csv, or a psv? Still csv is actually the only standard, which can be any character delimited (some say csv means character-separated-values), so one repo might use it different than another. Yet tsv and psv are csv files yet kind of hint at the delimiter in their extension.

[richmahn]

From a coworker:

The history of this puzzles me a little @rich Mahn. If I was writing code to autodetect TSV vs CSV, I'd look at the first/header line. And that line only has the column headers and tabs and zero commas. Why would it need to look further? How many lines was the old code scanning?

I do wonder if the first row, whether a header or not, is enough to go by, rather than counting all tabs and commas and other delimiters ALL the content as it currently does. You'd think that would be using more tabs than commas, or commas than tabs especially if it is a real header. But even if not, seems like still the best way to score the characters.

Edit: I see now it scores based on the first 10 lines, yet as my example has, my 2nd row actually has a bunch of commas in the text. Headers wouldn't normally do that.

[richmahn]

@lunny @silverwind

I suggest two things:

1. get the file extension passed to the Renderer so it can go by that if it isn't just .csv

2. When it is .csv, to guess the delimiter, we could do it from the first 10k or 10 rows, but why not then quickly see if the top scoring one matches with all 10 rows with the same number of fields, if not, the 2nd, 3rd, or 4th (assuming score > 0) and use the one that works, and if none do, return the top scoring one. If all have no score, then return , (comma). (how well it matches each row can be part of the score)

Thoughts?

[lunny]

I agree both. We can read ten lines to detect the delimiter. But one we need notice that some column have been quoted with double quote and contains delimiter characters in the double quote. i.e.

a,b,"c,b,d,",e

It should be 4 columns but not 7. If we just simple count single quote, that will be wrong.

[richmahn]

@lunny yeah, I thought of that and well aware of that. I assume to actually use the encoding/cvs module with those 10 lines and the delimiter in question and see what it returns. We'd still be making a best guess, as some rows might have the wrong number of fields, but if it has a higher accuracy than other delimiters, will use that delimiter. That is why we can kind of do a score with the matching of the delimiter as well.

[richmahn]

So just to be clear, making a fix for this that should also work with the CSV diff feature which has the same bug.

[zeripath]

Why don't we provide a mechanism for choosing whether to guess the delimiter or not.

[richmahn]

@zeripath So some sort of user interface the repo file view page? Yet would still have to guess something when first showing the file.

[richmahn]

@zeripath Also, the DIFF view for .csv files has this bug, and thus you'd have to have a selection for each file diff.

[silverwind]

Why don't we provide a mechanism for choosing whether to guess the delimiter or not.

I wouldn't. This is a kind of thing that can be handled automatically and should not need configuration for the most common types (csv,tsv,psv).

#16558 (comment) sounds good to me.

[zeripath]

Every heuristic mechanism will get things wrong for some pathological case.

[richmahn]

I still need to look at this. Will see if I can work on this. Found another bug with CSV diffs in that if the diff is after 32 lines, it fails to show the CSV table.