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

CSV previewing #4810

Closed
wants to merge 15 commits into
base: master
from

Conversation

6 participants
@Floppy

Floppy commented Aug 19, 2013

This is WIP code that will allow useful viewing and diffing of CSV files within Gitlab. It uses coopyhx to show the diffs.

Parts that still need work:

  • use same viewer for both file and diff view, for consistency
  • make Blob#to_csv more robust to different CSV variants
  • move Blob extensions into gitlab_git and github/linguist gems
  • improve naming of things in Javascript code - currently stolen straight from demo page
@Floppy

This comment has been minimized.

Show comment
Hide comment
@Floppy

Floppy Aug 19, 2013

File preview looks like:

csv_view

And diff view looks like:

csv_diff

Floppy commented Aug 19, 2013

File preview looks like:

csv_view

And diff view looks like:

csv_diff

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Aug 19, 2013

Coverage Status

Coverage remained the same when pulling ca6b3d1 on theodi:csv-view into a410bc1 on gitlabhq:master.

coveralls commented Aug 19, 2013

Coverage Status

Coverage remained the same when pulling ca6b3d1 on theodi:csv-view into a410bc1 on gitlabhq:master.

@paulfitz

This comment has been minimized.

Show comment
Hide comment
@paulfitz

paulfitz Aug 22, 2013

I'd be excited to see this in Gitlab. I could see trimming the amount of code needed, if that were a concern. One way would be by removing the handsontable widget, and instead producing a diff with something like:

var diff2html = new coopy.DiffRender();
diff2html.render(table_diff);
var table_diff_html = diff2html.html();

table_diff_html will be a regular html table code, suitable for sticking within a div with the highlighter class. It will have a similar appearance to the handsontable widget, but will not be editable (which doesn't seem needed here). At that point everything related to handsontable could be dropped.
The coopyhx code could also be trimmed too if needed. For example, it has logic for applying tabular patches, which isn't needed here. I'd be happy to refactor how coopyhx is released if that were helpful for uses like this.

paulfitz commented Aug 22, 2013

I'd be excited to see this in Gitlab. I could see trimming the amount of code needed, if that were a concern. One way would be by removing the handsontable widget, and instead producing a diff with something like:

var diff2html = new coopy.DiffRender();
diff2html.render(table_diff);
var table_diff_html = diff2html.html();

table_diff_html will be a regular html table code, suitable for sticking within a div with the highlighter class. It will have a similar appearance to the handsontable widget, but will not be editable (which doesn't seem needed here). At that point everything related to handsontable could be dropped.
The coopyhx code could also be trimmed too if needed. For example, it has logic for applying tabular patches, which isn't needed here. I'd be happy to refactor how coopyhx is released if that were helpful for uses like this.

@Floppy

This comment has been minimized.

Show comment
Hide comment
@Floppy

Floppy Aug 22, 2013

Thanks @paulfitz, that's great. I've changed the code to remove handsontable, and some other stuff that I realised wasn't necessary either.

I've found a couple of bugs in coopy.DiffRender(), which I'll file on your issue tracker.

New screenshot of diff:

diff

Floppy commented Aug 22, 2013

Thanks @paulfitz, that's great. I've changed the code to remove handsontable, and some other stuff that I realised wasn't necessary either.

I've found a couple of bugs in coopy.DiffRender(), which I'll file on your issue tracker.

New screenshot of diff:

diff

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Aug 22, 2013

Coverage Status

Coverage decreased (-0.04%) when pulling 8171fb1 on theodi:csv-view into a410bc1 on gitlabhq:master.

coveralls commented Aug 22, 2013

Coverage Status

Coverage decreased (-0.04%) when pulling 8171fb1 on theodi:csv-view into a410bc1 on gitlabhq:master.

@Floppy

This comment has been minimized.

Show comment
Hide comment
@Floppy

Floppy Aug 22, 2013

This needs fixes for paulfitz/daff#2 and paulfitz/daff#3, ideally.

Floppy commented Aug 22, 2013

This needs fixes for paulfitz/daff#2 and paulfitz/daff#3, ideally.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Aug 22, 2013

Coverage Status

Coverage decreased (-12.48%) when pulling 405fc79 on theodi:csv-view into a410bc1 on gitlabhq:master.

coveralls commented Aug 22, 2013

Coverage Status

Coverage decreased (-12.48%) when pulling 405fc79 on theodi:csv-view into a410bc1 on gitlabhq:master.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Aug 22, 2013

Coverage Status

Coverage remained the same when pulling 405fc79 on theodi:csv-view into a410bc1 on gitlabhq:master.

coveralls commented Aug 22, 2013

Coverage Status

Coverage remained the same when pulling 405fc79 on theodi:csv-view into a410bc1 on gitlabhq:master.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Aug 22, 2013

Coverage Status

Coverage decreased (-0.21%) when pulling d5dc8e7 on theodi:csv-view into 6683584 on gitlabhq:master.

coveralls commented Aug 22, 2013

Coverage Status

Coverage decreased (-0.21%) when pulling d5dc8e7 on theodi:csv-view into 6683584 on gitlabhq:master.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Aug 23, 2013

Coverage Status

Coverage decreased (-12.71%) when pulling 9e49eca on theodi:csv-view into 6683584 on gitlabhq:master.

coveralls commented Aug 23, 2013

Coverage Status

Coverage decreased (-12.71%) when pulling 9e49eca on theodi:csv-view into 6683584 on gitlabhq:master.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Aug 23, 2013

Coverage Status

Coverage decreased (-12.71%) when pulling b21af5a on theodi:csv-view into 6683584 on gitlabhq:master.

coveralls commented Aug 23, 2013

Coverage Status

Coverage decreased (-12.71%) when pulling b21af5a on theodi:csv-view into 6683584 on gitlabhq:master.

Floppy added some commits Aug 23, 2013

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Aug 23, 2013

Coverage Status

Coverage decreased (-12.67%) when pulling 8f72cdf on theodi:csv-view into 6683584 on gitlabhq:master.

coveralls commented Aug 23, 2013

Coverage Status

Coverage decreased (-12.67%) when pulling 8f72cdf on theodi:csv-view into 6683584 on gitlabhq:master.

@drdran

This comment has been minimized.

Show comment
Hide comment
@drdran

drdran Nov 18, 2013

Excuse me that feature will be included in the community version of gitlab?

drdran commented Nov 18, 2013

Excuse me that feature will be included in the community version of gitlab?

@Floppy

This comment has been minimized.

Show comment
Hide comment
@Floppy

Floppy Nov 18, 2013

I hope so, sometime, but that's not my call :)

I now have code that does this server-side, which is much more useful. Update to this PR will be coming soon...

Floppy commented Nov 18, 2013

I hope so, sometime, but that's not my call :)

I now have code that does this server-side, which is much more useful. Update to this PR will be coming soon...

@jvanbaarsen

This comment has been minimized.

Show comment
Hide comment
@jvanbaarsen

jvanbaarsen Dec 10, 2013

Contributor

@Floppy Are you still working on this?

Contributor

jvanbaarsen commented Dec 10, 2013

@Floppy Are you still working on this?

@Floppy

This comment has been minimized.

Show comment
Hide comment
@Floppy

Floppy Dec 10, 2013

I am, when I get the opportunity. I may have time next week to add the pure-Ruby implementation of the diffs, which is much better than doing it client-side.

Floppy commented Dec 10, 2013

I am, when I get the opportunity. I may have time next week to add the pure-Ruby implementation of the diffs, which is much better than doing it client-side.

@jvanbaarsen

This comment has been minimized.

Show comment
Hide comment
@jvanbaarsen

jvanbaarsen Dec 10, 2013

Contributor

Ok 😄

Contributor

jvanbaarsen commented Dec 10, 2013

Ok 😄

@jvanbaarsen

This comment has been minimized.

Show comment
Hide comment
@jvanbaarsen

jvanbaarsen Jan 13, 2014

Contributor

@Razer6 Maybe we can close this one untill the author has time to finish it?

Contributor

jvanbaarsen commented Jan 13, 2014

@Razer6 Maybe we can close this one untill the author has time to finish it?

@Razer6

This comment has been minimized.

Show comment
Hide comment
@Razer6

Razer6 Jan 13, 2014

Member

Yeap it's maybe a good idea. Open a new one when it's ready

Member

Razer6 commented Jan 13, 2014

Yeap it's maybe a good idea. Open a new one when it's ready

@Razer6 Razer6 closed this Jan 13, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment