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

CSVFiles: make delim a keyword argument in load #35

Closed
wants to merge 1 commit into from

Conversation

zweiglimmergneis
Copy link

See issue #16.

load accepts delim as a keyword argument for CSV files and/or TSV files. The delimiting character may also be specified as the second positional argument (as before).

Tests:

load(File(format"CSV", "../../data/Iris/iris.data"))
# ok

load(File(format"CSV", "../../data/Iris/iris.data"), ',')
#ok

load(File(format"CSV", "../../data/Iris/iris.data"), delim=',')
#ok

load(File(format"CSV", "../../data/Iris/iris.data"), skiplines_begin=3, header_exists=false)
# ok

load(File(format"CSV", "../../data/Iris/iris.data"), delim=',', skiplines_begin=3, header_exists=false)
# ok

load(File(format"CSV", "../../data/Iris/iris.data"), skiplines_begin=3, header_exists=false, delim=',')
# ok
# similar tests with a file containing ';' as a separator are successful, too

# tsv
load("../../data/Iris/iris.tsv")
# ok

load("../../data/Iris/iris.tsv", '\t')
# ok

load("../../data/Iris/iris.tsv", delim='\t')
# ok

When load is called with a stream as first argument, delim is accepted as a keyword argument. However, I could not extract data from a stream obtained with open:

     julia> csio = open("../../data/Iris/iris.csv")
     IOStream(<file ../../data/Iris/iris.csv>)

     julia> csvs = load(Stream(format"CSV", csio), skiplines_begin=3, header_exists=false);

     julia> df = DataFrame(csvs)
     ERROR: MethodError: no method matching String(::Ptr{UInt8}, ::Int64)
     Stacktrace:
      [1] #csvread#17(::Base.Iterators.Pairs{Symbol,Integer,Tuple{Symbol,Symbol},NamedTuple{(:skiplines_begin, :header_exists),Tuple{Int64,Bool}}}, ::Function, ::IOStream, ::Char) at /home/nepomuk/.julia/packages/TextParse/WFgcL/src/csv.jl:80
      [2] (::getfield(TextParse, Symbol("#kw##csvread")))(::NamedTuple{(:skiplines_begin, :header_exists),Tuple{Int64,Bool}}, ::typeof(TextParse.csvread), ::IOStream, ::Char) at ./none:0
      [3] getiterator(::CSVFiles.CSVStream) at /home/nepomuk/projekte/run/julia/CSVFiles/src/CSVFiles.jl:93
      [4] DataFrame(::CSVFiles.CSVStream) at /home/nepomuk/.julia/packages/DataFrames/VC5qi/src/other/tables.jl:22
      [5] top-level scope at none:0

See issue queryverse#16.

`load` accepts `delim` as a keyword argument for CSV files and/or TSV files. The delimiting character may also be specified as the second positional argument (as before).
@codecov-io
Copy link

codecov-io commented Nov 13, 2018

Codecov Report

Merging #35 into master will decrease coverage by 32.18%.
The diff coverage is 16.66%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master      #35       +/-   ##
===========================================
- Coverage     100%   67.81%   -32.19%     
===========================================
  Files           2        2               
  Lines          59       87       +28     
===========================================
  Hits           59       59               
- Misses          0       28       +28
Impacted Files Coverage Δ
src/CSVFiles.jl 60% <16.66%> (-40%) ⬇️
src/csv_writer.jl 76.19% <0%> (-23.81%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 153a21a...3a87378. Read the comment docs.

@davidanthoff
Copy link
Member

Great!

Maybe we could add some tests? And then I'll merge.

The stream issue looks more like a generic problem, do you also get that with the released version?

Oh, and then I'm wondering whether we should actually deprecate the positional version? I'm not sure...

@davidanthoff
Copy link
Member

I just merged another PR that implements this, should be in the tagged version now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants