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

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

@lafriks lafriks added type/enhancement An improvement of existing functionality topic/ui Change the appearance of the Gitea UI labels Feb 12, 2021
routers/repo/compare.go Show resolved Hide resolved
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Feb 12, 2021
services/gitdiff/csv.go Outdated Show resolved Hide resolved
services/gitdiff/csv.go Outdated Show resolved Hide resolved
@KN4CK3R
Copy link
Member Author

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
modules/base/csv.go Outdated Show resolved Hide resolved
@KN4CK3R KN4CK3R changed the title Feature CSV Diff Add Tabular Diff for CSV files Mar 6, 2021
Copy link
Contributor

@kdumontnu kdumontnu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome. Thanks for adding unit tests! 💯

@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Mar 25, 2021
@kdumontnu
Copy link
Contributor

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
Member

lunny commented Mar 25, 2021

@zeripath need your review.

@KN4CK3R KN4CK3R mentioned this pull request Mar 28, 2021
3 tasks
@zeripath zeripath dismissed their stale review March 28, 2021 19:07

concerns addressed

@6543
Copy link
Member

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 ...

modules/csv/csv.go Outdated Show resolved Hide resolved
modules/csv/csv.go Outdated Show resolved Hide resolved
@KN4CK3R
Copy link
Member Author

KN4CK3R commented Mar 29, 2021

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

@lafriks lafriks requested a review from kdumontnu March 29, 2021 20:01
@lafriks lafriks added the type/changelog Adds the changelog for a new Gitea version label Mar 29, 2021
@lafriks
Copy link
Member

lafriks commented Mar 29, 2021

Sorry accidentally requested review 😸

@6543 6543 merged commit 0c61376 into go-gitea:master Mar 29, 2021
@KN4CK3R KN4CK3R deleted the feature-csv-diff branch April 6, 2021 20:39
@go-gitea go-gitea locked and limited conversation to collaborators May 13, 2021
@delvh delvh removed the type/changelog Adds the changelog for a new Gitea version label Oct 7, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. topic/ui Change the appearance of the Gitea UI type/enhancement An improvement of existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants