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

Minimal call now fails #42

Open
eddelbuettel opened this issue Dec 21, 2021 · 6 comments
Open

Minimal call now fails #42

eddelbuettel opened this issue Dec 21, 2021 · 6 comments
Labels
bug 💣 Bug to fix short term ⏰ TODO short term

Comments

@eddelbuettel
Copy link
Contributor

Release 0.4.1 is really sweet but I fear this is a regression:

> plot_dependencies(deepdep("data.table"))
Error in `[.data.frame`(g, , "labeled") : undefined columns selected
> 

With the default of strong we can get zero row data.frame objects, and the plotting function does not like those.

@ErdaradunGaztea ErdaradunGaztea added bug 💣 Bug to fix short term ⏰ TODO short term labels Dec 21, 2021
@ErdaradunGaztea ErdaradunGaztea added this to the deepdep v0.4.2 milestone Dec 21, 2021
@ErdaradunGaztea
Copy link
Collaborator

Uhh, this one is worrying since it turned out it doesn't fail until it's plotted to a device. Precisely, saving above to a variable doesn't raise an exception, displaying it does:

# Perfectly fine
plt <- plot_dependencies("data.table")
# ggplot2 starts evaluating data and all fails suddenly
plt

and our tests don't do the latter.

This time, we won't be releasing fixed version immediately, mainly because of CRAN having winter break and not exactly liking our constant resubmissions. Also, I want to have a deeper look at the code before releasing, since it's like a fifth bug you submitted and that's enough embarrassment for me.

@ErdaradunGaztea ErdaradunGaztea pinned this issue Dec 21, 2021
@eddelbuettel
Copy link
Contributor Author

eddelbuettel commented Dec 21, 2021

and not exactly liking our constant resubmissions

I hear you. I try to space mine to not hit more than six resubmission in seven months (which means saying no to Conrad on some Armadillo updates). Most of my other repos move slower these days. That said, having the ability to have "local" (non-CRAN) releases is good. I eat my own dog food here as I have a few drat created repos (one overall for anything, one more for just Rcpp things). These days you also have Jeroen's r-universe, and of course the ability to build from git -- but I remain firmly in the 'mark a release, create a tarball, provide the release tarball' camp than on binaries or tarballs from each commit as he does which I still think is a little too close to the fire.

That said, the problem your teams is tackling is hard as there are so many different dependency schemes and outcomes and plot combinations. Plus you are too nice too me and have too many plot options :) Anyway, if you have a fix to test I'd be happy to git pull and install. And could run with that for a week or two or more before you go back to CRAN. And yes, now we have the winter break anyway.

@ErdaradunGaztea
Copy link
Collaborator

It's basically two-line fix and I pushed it to fix/no-deps branch, if you'd need it. And it's true about too many plot options, the way they interact with each other almost makes a mess of the code logic.

By the way, thanks for sharing your knowledge about drat, something nice to learn!

@eddelbuettel
Copy link
Contributor Author

Thanks, worked like a charm.

[ I am still partial to incrementing version numbers. I'd call this 0.4.1.1 (maybe once merged to mainline); I have no scheme for branches. ]

@ErdaradunGaztea
Copy link
Collaborator

(right, usethis suggests calling use_dev_version() after publishing a new release, so it's 0.4.1.9000 now)

@eddelbuettel
Copy link
Contributor Author

(Not a big fan of usethis and its overly "opionated" approaches but whatever floats your boat. I prefer 0.4.1.x with a single integer x that gets incremented. Many other projects do too, but hey some "opionated" people needed to invent a new scheme. It is different from the released version which is what matters.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 💣 Bug to fix short term ⏰ TODO short term
Projects
None yet
Development

No branches or pull requests

2 participants