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

Fix failing tests as a result of new readr #50

Merged
merged 2 commits into from
Feb 9, 2019
Merged

Conversation

pralitp
Copy link
Contributor

@pralitp pralitp commented Feb 5, 2019

Update comparison data and fix test for the new version of readr's column type behavior.

@pralitp
Copy link
Contributor Author

pralitp commented Feb 5, 2019

The remaining test failures are having to do with tidyverse/readr#963. We could either: Ignore the failures for now and see if the fix gets merged. Or since readr now:

We now write the connection to a temporary file (in the R temporary directory), than parse that temporary file.

We probably no longer see the performance benefit from using pipe instead of system2. We could just manage writing to the temporary file ourselves and we could sidestep the readr bug. We could then also get better error messages as well. Although this really applies to local DB connections and remote DB connections may still suffer from the bug (I'm not sure since it calls readr indirectly via httr package).

@pralitp pralitp requested a review from rplzzz February 5, 2019 19:10
@rplzzz
Copy link
Contributor

rplzzz commented Feb 5, 2019

Is this bug causing people problems right now? If not, then we can probably wait to see if they merge a fix. If it is, or if the fix takes too long, then we should probably devise our own workaround.

@pralitp
Copy link
Contributor Author

pralitp commented Feb 6, 2019

No, I don't think this bug is going to really cause any rgcam users any real trouble. It is really only about graceful error messages so the worst that can happen is we get a question about what does "Cannot read file" error mean?.

And I don't really have to time to deal with it so I am just going to punt for now.

@codecov
Copy link

codecov bot commented Feb 6, 2019

Codecov Report

Merging #50 into master will decrease coverage by 0.7%.
The diff coverage is n/a.

Impacted Files Coverage Δ
R/querymi.R 84.54% <0%> (-1.82%) ⬇️
R/manageProjectData.R 88.97% <0%> (-0.79%) ⬇️

2 similar comments
@codecov
Copy link

codecov bot commented Feb 6, 2019

Codecov Report

Merging #50 into master will decrease coverage by 0.7%.
The diff coverage is n/a.

Impacted Files Coverage Δ
R/querymi.R 84.54% <0%> (-1.82%) ⬇️
R/manageProjectData.R 88.97% <0%> (-0.79%) ⬇️

@codecov
Copy link

codecov bot commented Feb 6, 2019

Codecov Report

Merging #50 into master will decrease coverage by 0.7%.
The diff coverage is n/a.

Impacted Files Coverage Δ
R/querymi.R 84.54% <0%> (-1.82%) ⬇️
R/manageProjectData.R 88.97% <0%> (-0.79%) ⬇️

Copy link
Contributor

@rplzzz rplzzz left a comment

Choose a reason for hiding this comment

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

Works for me.

:shipit:

@pralitp pralitp merged commit 57ffb46 into master Feb 9, 2019
@pralitp pralitp deleted the fix-readr-1.3.1 branch February 9, 2019 15:32
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.

2 participants