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

CompatHelper: bump compat for CSV to 0.9, (keep existing compat) #550

Conversation

github-actions[bot]
Copy link
Contributor

@github-actions github-actions bot commented Sep 8, 2021

This pull request changes the compat entry for the CSV package from 0.8 to 0.8, 0.9.
This keeps the compat entries for earlier versions.

Note: I have not tested your package with this new compat entry.
It is your responsibility to make sure that your package tests pass before you merge this pull request.

@Libbum
Copy link
Member

Libbum commented Sep 8, 2021

Tests fail on 0.9.

CSV: Error During Test at /mnt/turtle/repos/Agents.jl/test/csv_tests.jl:1
  Got exception outside of a @test
  ArgumentError: invalid column name provided in `types` keyword argument: id. Valid column names detected in the data are: [:old_opinion]

@AayushSabharwal
Copy link
Collaborator

Interesting. I'll take a look

@AayushSabharwal
Copy link
Collaborator

AayushSabharwal commented Sep 11, 2021

This PR: JuliaData/CSV.jl#870 Seems to be the cause of this. CSV.jl now validates the types keyword to make sure each key exists as a column in the file. It looks like the check is unavoidable: https://github.com/JuliaData/CSV.jl/blob/a9eefcda332c25cd9224773842f49cbf549b0cf0/src/context.jl#L432. I am trying to see if there is a way to get the column names detected by CSV.Rows, so types can be filtered accordingly.

@Libbum
Copy link
Member

Libbum commented Sep 12, 2021

Think we'd be fine if we need to move to 0.9 as our base and drop support for 0.8 if you can't see an easy solution.

@AayushSabharwal
Copy link
Collaborator

It's the other way around. That PR was merged in 0.9, so the current setup only works with 0.8.

@AayushSabharwal
Copy link
Collaborator

I can't find a way to do this with 0.9. I'll open an issue in CSV.jl, since this seems like something that should be optional.

@AayushSabharwal
Copy link
Collaborator

I passed validate = false which was added in CSV.jl v0.9.6. This, however, means we can't support anything before it (since the keyword doesn't exist). Right now the compat entry is at 0.9 (I removed 0.8). I also encountered a weird bug inside CSV.jl locally, so waiting for tests to run here and see if it's just me.

@AayushSabharwal
Copy link
Collaborator

AayushSabharwal commented Oct 15, 2021

This looks like it specifically happens when the type of a column is inferred to be Symbol.
The CSV file generated in csv_tests.jl:89:

id,pos_1,pos_2,pos_3,has_prisoner,capture_time,shape
5,11,86,1,false,0,diamond
4,5,43,1,false,0,diamond
6,16,90,1,false,0,diamond
7,75,35,1,false,0,diamond
2,35,89,1,false,0,diamond
10,81,25,1,false,0,diamond
9,48,98,1,false,0,diamond
8,98,62,1,false,0,diamond
3,50,2,1,false,0,diamond
1,95,24,1,false,0,diamond

If the type of :shape is specified to be String, CSV.Rows works. However, when specifying Symbol it errors.
works:

for r in CSV.Rows(read("test/test.csv"); types = Dict(:id => Int, :pos_1 => Int, :pos_2 => Int, :pos_3 => Int, :has_prisoner => Bool, :capture_time => Int, :shape => String))
       print(r)
end

doesn't work:

for r in CSV.Rows(read("test/test.csv"); types = Dict(:id => Int, :pos_1 => Int, :pos_2 => Int, :pos_3 => Int, :has_prisoner => Bool, :capture_time => Int, :shape => Symbol))
       print(r)
end

The same error does not occur for CSV.File. I'll open an issue in CSV.jl

@AayushSabharwal
Copy link
Collaborator

AayushSabharwal commented Oct 22, 2021

Tests pass locally now, I'm not sure how to get it to rerun CI here without pushing a couple fake commits (comment-uncomment a line)

EDIT: Found it :)

@codecov-commenter
Copy link

codecov-commenter commented Oct 22, 2021

Codecov Report

Merging #550 (27dbc43) into master (1390959) will not change coverage.
The diff coverage is 100.00%.

❗ Current head 27dbc43 differs from pull request most recent head 92118a6. Consider uploading reports for the commit 92118a6 to get more accurate results
Impacted file tree graph

@@           Coverage Diff           @@
##           master     #550   +/-   ##
=======================================
  Coverage   94.37%   94.37%           
=======================================
  Files          24       24           
  Lines        1457     1457           
=======================================
  Hits         1375     1375           
  Misses         82       82           
Impacted Files Coverage Δ
src/submodules/io/csv_integration.jl 100.00% <100.00%> (ø)

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 1390959...92118a6. Read the comment docs.

Copy link
Member

@Datseris Datseris left a comment

Choose a reason for hiding this comment

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

@AayushSabharwal feel free to merge here.

@Datseris
Copy link
Member

Datseris commented Nov 4, 2021

Oh, but first please bump the CSV version to its appripriate one (I think 0.9.6?)

@AayushSabharwal AayushSabharwal merged commit 1280509 into master Nov 5, 2021
@AayushSabharwal AayushSabharwal deleted the compathelper/new_version/2021-09-08-05-15-15-607-01521946810 branch November 5, 2021 10:44
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.

4 participants