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 /_data/*.tsv support #5985

Merged
merged 13 commits into from Mar 28, 2017

Conversation

Projects
None yet
8 participants
@Crissov
Contributor

Crissov commented Mar 27, 2017

  • fixes #5979 (or so I hope, I'm new to Ruby)
  • add support for .tsv with tab separated columns in _data/ folder
  • not adding support for auto-detecting :col_sep in .csv

https://help.github.com/articles/rendering-csv-and-tsv-data/
ftp://ftp.iana.org/assignments/media-types/text/tab-separated-values
https://www.ietf.org/rfc/rfc4180.txt (CSV)
https://ruby-doc.org/stdlib-2.4.1/libdoc/csv/rdoc/CSV.html

@DirtyF

This comment has been minimized.

Show comment
Hide comment
@DirtyF
Member

DirtyF commented Mar 27, 2017

@DirtyF DirtyF added the enhancement label Mar 27, 2017

@mattr-

mattr- approved these changes Mar 27, 2017

@mattr-

This comment has been minimized.

Show comment
Hide comment
@mattr-

mattr- Mar 27, 2017

Member

The builds need to pass. Otherwise, LGTM.

Member

mattr- commented Mar 27, 2017

The builds need to pass. Otherwise, LGTM.

@pathawks

This comment has been minimized.

Show comment
Hide comment
@pathawks

pathawks Mar 27, 2017

Member

Is there a reason for the extra commits?

This would be easier to review if it were just changes in features/data.feature and lib/jekyll/readers/data_reader.rb

Member

pathawks commented Mar 27, 2017

Is there a reason for the extra commits?

This would be easier to review if it were just changes in features/data.feature and lib/jekyll/readers/data_reader.rb

@Crissov

This comment has been minimized.

Show comment
Hide comment
@Crissov

Crissov Mar 27, 2017

Contributor

@pathawks I accidentally did a rebase, because I'm still not completely understanding the git philosophy, and don't know how to revert that elegantly.

Contributor

Crissov commented Mar 27, 2017

@pathawks I accidentally did a rebase, because I'm still not completely understanding the git philosophy, and don't know how to revert that elegantly.

Update data.feature
don't do semicolons and tabs in .csv within this patch
@ashmaroli

This comment has been minimized.

Show comment
Hide comment
@ashmaroli

ashmaroli Mar 28, 2017

Member

@Crissov running the following (from your PR branch) should sort this out for you:

$ git fetch upstream master
$ git rebase upstream/master
Member

ashmaroli commented Mar 28, 2017

@Crissov running the following (from your PR branch) should sort this out for you:

$ git fetch upstream master
$ git rebase upstream/master

Crissov added some commits Mar 28, 2017

Merge pull request #4 from Crissov/master
update from master
@Crissov

This comment has been minimized.

Show comment
Hide comment
@Crissov

Crissov Mar 28, 2017

Contributor

@ashmaroli I currently only have Web access to the repository, but tried my best to do that.

Contributor

Crissov commented Mar 28, 2017

@ashmaroli I currently only have Web access to the repository, but tried my best to do that.

@ashmaroli

This comment has been minimized.

Show comment
Hide comment
@ashmaroli

ashmaroli Mar 28, 2017

Member

The diff is clean enough for a review now. You can update the branch as and when you have access to your local system.

Member

ashmaroli commented Mar 28, 2017

The diff is clean enough for a review now. You can update the branch as and when you have access to your local system.

@Crissov

This comment has been minimized.

Show comment
Hide comment
@Crissov

Crissov Mar 28, 2017

Contributor

I'm not sure why the automated tests are failing. Do .feature files support literal tab characters or do they have to be encoded as \t or something else?

Contributor

Crissov commented Mar 28, 2017

I'm not sure why the automated tests are failing. Do .feature files support literal tab characters or do they have to be encoded as \t or something else?

Show outdated Hide outdated features/data.feature
"""
name age
Jack 28
Leon 34

This comment has been minimized.

@Crissov

Crissov Mar 28, 2017

Contributor

Does this have to be Leon\t34?

@Crissov

Crissov Mar 28, 2017

Contributor

Does this have to be Leon\t34?

This comment has been minimized.

@ashmaroli

ashmaroli Mar 28, 2017

Member

No, that's not required. I pulled your branch locally to check. Its seems you delimited by 2 \s (2 space) characters instead of a tab character

@ashmaroli

ashmaroli Mar 28, 2017

Member

No, that's not required. I pulled your branch locally to check. Its seems you delimited by 2 \s (2 space) characters instead of a tab character

This comment has been minimized.

@Crissov

Crissov Mar 28, 2017

Contributor

🤦

@Crissov

Crissov Mar 28, 2017

Contributor

🤦

Update data.feature
I don't know which component replaced my tab characters by space before.
Show outdated Hide outdated features/data.feature
@@ -9,7 +9,7 @@ Feature: Data
"""
- name: sugar
price: 5.3
- name: salt
- name: sal
price: 2.5

This comment has been minimized.

@ashmaroli

ashmaroli Mar 28, 2017

Member

don't nick the t 😄

@ashmaroli

ashmaroli Mar 28, 2017

Member

don't nick the t 😄

This comment has been minimized.

@Crissov

Crissov Mar 28, 2017

Contributor

Fixed. Not sure how that happened.

@Crissov

Crissov Mar 28, 2017

Contributor

Fixed. Not sure how that happened.

@oe

This comment has been minimized.

Show comment
Hide comment
@oe

oe Mar 28, 2017

Member

LGTM, test failures are unrelated

Member

oe commented Mar 28, 2017

LGTM, test failures are unrelated

@oe

oe approved these changes Mar 28, 2017

@ashmaroli

This comment has been minimized.

Show comment
Hide comment
@ashmaroli

ashmaroli Mar 28, 2017

Member

LGTM, test failures are unrelated

Actually, the following is related to this PR:

lib/jekyll/readers/data_reader.rb:61:11: C: Style/AlignHash: Align the elements of a hash literal if they span more than one line.
          :col_sep => "\t",
          ^^^^^^^^^^^^^^^^
Member

ashmaroli commented Mar 28, 2017

LGTM, test failures are unrelated

Actually, the following is related to this PR:

lib/jekyll/readers/data_reader.rb:61:11: C: Style/AlignHash: Align the elements of a hash literal if they span more than one line.
          :col_sep => "\t",
          ^^^^^^^^^^^^^^^^
@parkr

I'd also like to see a test in minitest (test/*.rb) as well.

Show outdated Hide outdated lib/jekyll/readers/data_reader.rb
@@ -56,6 +56,12 @@ def read_data_file(path)
:headers => true,
:encoding => site.config["encoding"],
}).map(&:to_hash)
when ".tsv"
CSV.read(path, {
:col_sep => "\t",

This comment has been minimized.

@parkr

parkr Mar 28, 2017

Member

@ashmaroli pointed out that rubocop correctly flagged this as incorrectly spaced. Run script/fmt -a to auto-correct it.

@parkr

parkr Mar 28, 2017

Member

@ashmaroli pointed out that rubocop correctly flagged this as incorrectly spaced. Run script/fmt -a to auto-correct it.

This comment has been minimized.

@Crissov

Crissov Mar 28, 2017

Contributor

done

@Crissov

Crissov Mar 28, 2017

Contributor

done

Update data_reader.rb
add a single space to satisfy format checker
@pathawks

This comment has been minimized.

Show comment
Hide comment
@pathawks

pathawks Mar 28, 2017

Member

I wonder if we could squash these commits before merging.

Member

pathawks commented Mar 28, 2017

I wonder if we could squash these commits before merging.

@Crissov

This comment has been minimized.

Show comment
Hide comment
@Crissov

Crissov Mar 28, 2017

Contributor

@parkr I'm not quite sure what exactly you are expecting, since I don't see anything similar related to CSV. Could you please elaborate?
@pathawks You can make it a squash merge, no?

Contributor

Crissov commented Mar 28, 2017

@parkr I'm not quite sure what exactly you are expecting, since I don't see anything similar related to CSV. Could you please elaborate?
@pathawks You can make it a squash merge, no?

@oe

This comment has been minimized.

Show comment
Hide comment
@oe

oe Mar 28, 2017

Member

@Crissov sadly, our jekyllbot doesn't allow for squash merges (i think). a squash rebase should be easy enough. maybe try following this guide?

Member

oe commented Mar 28, 2017

@Crissov sadly, our jekyllbot doesn't allow for squash merges (i think). a squash rebase should be easy enough. maybe try following this guide?

@pathawks

This comment has been minimized.

Show comment
Hide comment
@pathawks

pathawks Mar 28, 2017

Member

@Crissov If you need help, somebody else could squash once you're finished; that's not a problem.

Just wanted to make a note about it so that it happens before merge 🍻

Member

pathawks commented Mar 28, 2017

@Crissov If you need help, somebody else could squash once you're finished; that's not a problem.

Just wanted to make a note about it so that it happens before merge 🍻

@ashmaroli

This comment has been minimized.

Show comment
Hide comment
@ashmaroli

ashmaroli Mar 28, 2017

Member

@Crissov regarding what @parkr meant, this should make a nice example.

Member

ashmaroli commented Mar 28, 2017

@Crissov regarding what @parkr meant, this should make a nice example.

@parkr

parkr approved these changes Mar 28, 2017

@parkr parkr merged commit 3688640 into jekyll:master Mar 28, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

parkr added a commit that referenced this pull request Mar 28, 2017

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Mar 28, 2017

Member

Great work, @Crissov! Thanks. ❤️

Member

parkr commented Mar 28, 2017

Great work, @Crissov! Thanks. ❤️

@Crissov Crissov deleted the Crissov:TSV branch Mar 28, 2017

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