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.

Sign up for GitHub

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

#4148 add values_at method for CSV::Row #4157

Merged
merged 6 commits into from Mar 20, 2017
Merged

#4148 add values_at method for CSV::Row #4157

merged 6 commits into from Mar 20, 2017

Conversation

need47
Copy link
Contributor

@need47 need47 commented Mar 17, 2017

This PR adds the following methods to CSV::Row in (src/csv.cr)

values_at(*columns : Int)
values_at(*headers: String)

to retrieve multiple values at given columns or corresponding to headers.

A spec file was also provided at: spec/std/csv/csv_values_at_spec.cr

It's my first PR ever. If I was doing something wrong, please kindly let me know

Sija
Sija reviewed Mar 17, 2017
View reviewed changes
src/csv.cr Outdated
# Raises `IndexError` if any column doesn't exist
# The behavior of returning a tuple is similar to `Hash#values_at`
def values_at(*columns : Int)
columns.map { |column| self.[](column) }
Copy link
Contributor

Choose a reason for hiding this comment

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

self[column]

Sija
Sija reviewed Mar 17, 2017
View reviewed changes
src/csv.cr Outdated
# Raises `CSV::Error` if headers were not requested
# The behavior of returning a tuple is similar to `Hash#values_at`
def values_at(*headers : String)
headers.map { |header| self.[](header) }
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto

@need47
Copy link
Contributor Author

need47 commented Mar 17, 2017

@Sija I have updated the PR according to your comment. My local test was good, but I saw that Travis CI had failed. Not sure whether it's related to my PR. Let me know if there is anything I can do.

@bcardiff
Copy link
Member

Hi @need47, the failures are because of formatting.

If you run

$ crystal tool format spec/std/csv/csv_values_at_spec.cr
$ crystal tool format src/csv.cr

There should be changes to submit and will make CI pass. At least linux one. There are currently some issues regarding OSX CI.

@need47
Copy link
Contributor Author

need47 commented Mar 17, 2017

All checks passed except for OSX CI. Just FYI, I did compile LLVM (version 3.9.1) without the OSX platform.

@need47
Copy link
Contributor Author

need47 commented Mar 17, 2017

I am reading the spec/std/csv/csv_spec.cr and found an interesting use of CSV.

it "can do new with block" do
    CSV.new(%(one, two\n1, 2\n3, 4\n5), headers: true, strip: true) do |csv|
      csv["one"].should eq("1")
      csv["two"].should eq("2")
      break
    end
  end

a csv object can call [] directly as if it was a row. So shall I also add CSV#values_at() methods, in addition to CSV::Row#values_at() in this PR?

@bcardiff
Copy link
Member

@need47 Sure! mimic how row_internal is used in the other [] methods.

1) added CSV#values_at; 2) moved #values_at spec to spec/std/csv/csv_…
53b5eb4 
…spec.cr 3) removed the unnecessary file spec/std/csv/csv_values_at_spec.cr
@need47
Copy link
Contributor Author

need47 commented Mar 17, 2017

I have added CSV#values_at and CI passed.

@bcardiff bcardiff added this to the Next milestone Mar 17, 2017
@bcardiff
Copy link
Member

@need47 Sorry, I've just noticed that the specs should be at spec/std/csv/csv_spec.cr, would you move them when you have time?

@need47
Copy link
Contributor Author

need47 commented Mar 18, 2017

specs moved and CI passed.

@bcardiff bcardiff merged commit 72e626e into crystal-lang:master Mar 20, 2017
@bcardiff
Copy link
Member

Merged! Thanks @need47 for your first contribution to crystal

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

@Sija Sija Sija left review comments

Assignees
No one assigned
Labels
kind:feature topic:stdlib
Projects
None yet
Milestone
0.22.0
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@need47 @bcardiff @Sija