The Wayback Machine - https://web.archive.org/web/20220506231446/https://github.com/go-gitea/gitea/pull/14661
Skip to content
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

Jump to bottom

Add Tabular Diff for CSV files #14661

Merged
merged 30 commits into from Mar 29, 2021
Merged

Add Tabular Diff for CSV files #14661

merged 30 commits into from Mar 29, 2021

Conversation

KN4CK3R
Copy link
Member

@KN4CK3R KN4CK3R commented Feb 12, 2021

Implements request #14320 The rendering of csv files does match the diff style.

A simple change:
grafik

Changes in different areas (note the jumping line numbers):
grafik

A removed column is rendered without marking all rows as changed:
grafik

Toggle view:
toggle

zeripath
zeripath previously requested changes Feb 12, 2021
View changes
routers/repo/compare.go Show resolved Hide resolved
zeripath
zeripath reviewed Feb 12, 2021
View changes
services/gitdiff/csv.go Outdated Show resolved Hide resolved
zeripath
zeripath reviewed Feb 12, 2021
View changes
services/gitdiff/csv.go Outdated Show resolved Hide resolved
@KN4CK3R
Copy link
Author

@KN4CK3R KN4CK3R commented Feb 13, 2021

File sizes are now restricted to 512kb.
grafik

This is a problem for the old csv markup renderer because there is no way to return errors from the rendering process and fall back to the normal text rendering. If the file is too large the content is now rendered without styling:
grafik
With a fallback to the normal rendering combined with an error message from the markup renderer it could be displayed like on GitHub:
grafik

Additionaly the csv parser errors are now propagated to the ui:
grafik

@lunny lunny added this to the 1.15.0 milestone Feb 13, 2021
lunny
lunny reviewed Feb 13, 2021
View changes
modules/base/csv.go Outdated Show resolved Hide resolved
lunny
lunny reviewed Feb 15, 2021
View changes
modules/csv/csv.go Show resolved Hide resolved
lunny
lunny reviewed Feb 15, 2021
View changes
modules/csv/csv.go Show resolved Hide resolved
@KN4CK3R KN4CK3R mentioned this pull request Mar 3, 2021
Merged
@KN4CK3R KN4CK3R changed the title Feature CSV Diff Add Tabular Diff for CSV files Mar 6, 2021
kdumontnu
kdumontnu approved these changes Mar 25, 2021
View changes
Copy link
Contributor

@kdumontnu kdumontnu left a comment

Awesome. Thanks for adding unit tests! 💯

@kdumontnu
Copy link

@kdumontnu kdumontnu commented Mar 25, 2021

The problem is the empty line which is not handled as expected by the Go CSV reader (golang/go#39119). I don't think this PR could and should support all variations in CSV files out there. For usual field,field,field files it works as itended and that should be the majority of CSV files.

Agreed. I've added some support to the issue/PR you linked, but it appears that the go maintainers are not interested in extending the encode/csv library and suggest using other libraries. I might investigate other csv reader libraries.

@lunny
Copy link

@lunny lunny commented Mar 25, 2021

@zeripath need your review.

@KN4CK3R KN4CK3R mentioned this pull request Mar 28, 2021
Merged
3 tasks
@zeripath zeripath dismissed their stale review Mar 28, 2021

concerns addressed

@6543
Copy link

@6543 6543 commented Mar 28, 2021

looks ok, just found one dark-theme issue and one render issue:
Bildschirmfoto zu 2021-03-29 01-08-13
Bildschirmfoto zu 2021-03-29 01-08-37

on the Diff View the ® dont get displayed correctly, while on normal file-view it is ...
both views have an collor issue (white line), whitch should be hidden at all ...

lafriks
lafriks reviewed Mar 29, 2021
View changes
modules/csv/csv.go Outdated Show resolved Hide resolved
lafriks
lafriks reviewed Mar 29, 2021
View changes
modules/csv/csv.go Outdated Show resolved Hide resolved
@KN4CK3R
Copy link
Author

@KN4CK3R KN4CK3R commented Mar 29, 2021 •
edited

on the Diff View the ® dont get displayed correctly, while on normal file-view it is ...

Could you please provide the sample file? Looks like an encoding issue. I have added a call to charset.ToUTF8WithFallback like in view.go. There is an unneeded call too (line 485 and 498):

gitea/routers/repo/view.go

Lines 485 to 498 in 2b9e0b4

buf = charset.ToUTF8WithFallback(append(buf, d...))
readmeExist := markup.IsReadmeFile(blob.Name())
ctx.Data["ReadmeExist"] = readmeExist
if markupType := markup.Type(blob.Name()); markupType != "" {
ctx.Data["IsMarkup"] = true
ctx.Data["MarkupType"] = markupType
ctx.Data["FileContent"] = string(markup.Render(blob.Name(), buf, path.Dir(treeLink), ctx.Repo.Repository.ComposeDocumentMetas()))
} else if readmeExist {
ctx.Data["IsRenderedHTML"] = true
ctx.Data["FileContent"] = strings.ReplaceAll(
gotemplate.HTMLEscapeString(string(buf)), "\n", `<br>`,
)
} else {
buf = charset.ToUTF8WithFallback(buf)

Edit: I found the file and it looks good with the change.
grafik

6543
6543 approved these changes Mar 29, 2021
View changes
@lafriks lafriks requested a review from kdumontnu Mar 29, 2021
@lafriks
Copy link

@lafriks lafriks commented Mar 29, 2021

Sorry accidentally requested review 😸

@6543 6543 merged commit 0c61376 into go-gitea:master Mar 29, 2021
2 checks passed
@kdumontnu kdumontnu mentioned this pull request Mar 30, 2021
Closed
@KN4CK3R KN4CK3R deleted the feature-csv-diff branch Apr 6, 2021
@go-gitea go-gitea locked and limited conversation to collaborators May 13, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@zeripath zeripath

@bagasme bagasme

@lunny lunny

@lafriks lafriks

@kdumontnu kdumontnu

@6543 6543

Assignees
No one assigned
Labels
changelog kind/enhancement kind/ui lgtm/done
Milestone
1.15.0
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@KN4CK3R @kdumontnu @lunny @6543 @lafriks @zeripath @bagasme @GiteaBot