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

DataFrames fixes #1318

Merged
merged 1 commit into from
Sep 24, 2019
Merged

DataFrames fixes #1318

merged 1 commit into from
Sep 24, 2019

Conversation

Mattriks
Copy link
Member

@Mattriks Mattriks commented Sep 15, 2019

  • I've added an entry to NEWS.md
  • I've added and/or updated the unit tests
  • I've run the regression tests
  • I've built the docs and confirmed these changes don't cause new errors

This PR:

@tlnagy
Copy link
Member

tlnagy commented Sep 18, 2019

This looks great! And thanks for taking the time to update the tests to the new API. I think this'll be an important stopgap change until we update everything to work with the new Tables API (#1292). This is a more thorough update than either #1303 and #1313 so I will merge this one in lieu of those.

@tlnagy
Copy link
Member

tlnagy commented Sep 18, 2019

This update might preserve Julia 0.7 compatability

Did you try running the tests locally on Julia 0.7? I wasn't able to track down the xparse error (https://travis-ci.org/GiovineItalia/Gadfly.jl/jobs/585115113#L505) and master's tests pass for me locally on 0.7 (OpenSUSE Leap 15.1).

@Mattriks
Copy link
Member Author

I need to update the docs too, so don't merge yet.

@tlnagy
Copy link
Member

tlnagy commented Sep 18, 2019

I wasn't going to merge until it passes on 0.7 locally, but yeah, I missed the lack of doc updates. That's needed to truly fix #1304

@Mattriks
Copy link
Member Author

The xparse error is strange and appears to originate in Compose.jl here. There are also new deprecation warnings for Compose with Cairo 0.7, but that's a separate issue too.

@tlnagy
Copy link
Member

tlnagy commented Sep 18, 2019

Yeah, I tried tracking it down to this PR: JuliaIO/JSON.jl#285 (comment) when the switch to the new API was made, but the tests pass so I'm sure why it's failing in Compose's tests.

@codecov-io
Copy link

Codecov Report

Merging #1318 into master will decrease coverage by 0.03%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1318      +/-   ##
==========================================
- Coverage    90.1%   90.07%   -0.04%     
==========================================
  Files          36       36              
  Lines        3953     3940      -13     
==========================================
- Hits         3562     3549      -13     
  Misses        391      391
Impacted Files Coverage Δ
src/dataframes.jl 97.5% <100%> (+6.93%) ⬆️
src/Gadfly.jl 77.58% <0%> (-1.01%) ⬇️

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 0eed988...e33372f. Read the comment docs.

@Mattriks
Copy link
Member Author

I updated the docs, and they build locally, so this PR is ready to merge.

@Mattriks
Copy link
Member Author

I also tracked down the xparse error. It happens when testing with Julia 0.7, because Parsers.jl 0.2.22 gets installed. xparse is found in Parsers.jl 0.3 (not 0.2.22). So when testing on Julia 0.7, the incorrect version of Parsers.jl is getting installed. Note when testing Gadfly on Julia 1.x, Parsers.jl doesn't appear in the Resolving package version... listing.

@tlnagy
Copy link
Member

tlnagy commented Sep 22, 2019

So when testing on Julia 0.7, the incorrect version of Parsers.jl is getting installed.

I installed Julia 0.7 locally and all the tests passed, it's only on Travis that I saw the xparse error...

I updated the docs, and they build locally, so this PR is ready to merge.

awesome!

@Mattriks
Copy link
Member Author

Parsers 0.2.22 doesn't have an xparse function, so does that mean the Gadfly tests will pass locally on Julia 0.7 if certain lines in Compose are not being evaluated (depending on things like system font setup?)

@tlnagy
Copy link
Member

tlnagy commented Sep 24, 2019

Ah, that makes sense.

PR is ready to merge.

Lets do it.

@Mattriks
Copy link
Member Author

@tlnagy The dev docs haven't built.

@tlnagy
Copy link
Member

tlnagy commented Sep 25, 2019

That's due to the 0.7 builds failing (https://travis-ci.org/GiovineItalia/Gadfly.jl/jobs/589150627) and the doc build getting cancelled. I manually started the doc build, but that's not sustainable going forward.

We need to figure the xparse error on 0.7 or stop testing on Travis on 0.7.

@Mattriks
Copy link
Member Author

Re the first option: Parsers.jl 0.3 (which has xparse) is compatible with Julia 0.7, so what's holding it back at Parsers 0.2.22?

@tlnagy
Copy link
Member

tlnagy commented Sep 25, 2019

Not sure, what's holding it back. Might be one of our dependencies

@Mattriks
Copy link
Member Author

Mattriks commented Sep 25, 2019

In the Resolving package versions... are there packages that are being added purely to setup the test (by Travis or by Julia Pkg)?, i.e. are some of the listed packages not in any way dependencies of Gadfly?

@tlnagy
Copy link
Member

tlnagy commented Sep 25, 2019

These are the only packages that are installed for tests in addition to the ones Gadfly directly depends on.

Gadfly.jl/Project.toml

Lines 58 to 68 in aa4855c

[extras]
CSV = "336ed68f-0bac-5ca0-87d4-7b16caf5d00b"
Cairo = "159f3aea-2a34-519c-b102-8c37f9878175"
DataFrames = "a93c6f00-e57d-5684-b7b6-d8193f3e46c0"
LibGit2 = "76f85450-5226-5b5a-8eaa-529ad045b433"
RDatasets = "ce6b1742-4840-55fa-b093-852dadbb1d8b"
Test = "8dfed614-e22c-5e08-85e1-65c5234f0b40"
Unitful = "1986cc42-f94f-5a68-af5c-568840ba703d"
[targets]
test = ["CSV", "Cairo", "DataFrames", "LibGit2", "RDatasets", "Test", "Unitful"]

@tlnagy
Copy link
Member

tlnagy commented Sep 25, 2019

It looks like maybe this is caused by JuliaStats/RDatasets.jl#70 that dropped support for julia 0.7 and is maybe forcing the older version of CSV that requires the old Parsers?

I'm sure how to read the bounds exactly here: https://github.com/JuliaRegistries/General/blob/e69a570be9c66cbdd39c2d94e4e83e897f9000aa/R/RDatasets/Compat.toml#L52-L56

@Mattriks
Copy link
Member Author

Compat.toml bounds: JuliaLang/Pkg.jl#1190 (comment)

@tlnagy tlnagy mentioned this pull request Oct 9, 2019
@Mattriks Mattriks mentioned this pull request Nov 22, 2019
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tutorial not working (deprecation in dataframes.jl)
3 participants