search

thomasp85 / grid

personal devel version of grid
15 stars 4 forks source link

Guard against old unit objects #13

Closed thomasp85 closed 5 years ago

thomasp85 commented 5 years ago

@pmur002 this is my proposal for better errors when using old-style units with the new implementation. Rather than trying to detect specific instances of the old units I've elected to instead have the new unit class include a unit_v2 class at the top. is.unit() now checks for inheritance of unit_v2 and throws an error if an object inherits from unit but not unit_v2

I've also made changes to the functions that doesn't call is.unit(), some at the R level and some at the C level, to make sure that every call into the unit functionality is validated.

What do you think of this approach?

thomasp85 commented 5 years ago

btw, this branch derives from #1 so it includes a lot of unreleased commits... the relevant one is the last one (89a7445)

thomasp85 commented 5 years ago

Yeah, both this and #12 will be merged into #1

We have submitted the ggplot2 hot fix at the recommendation of Hadley but have yet to hear back (there were unrelated stochastic revdep problems during pretest), so hopefully the water will soon be clear to try again...

thomasp85 commented 5 years ago

If there are a few packages outside of ggplot2's reverse dependency tree that needs updating to work with the new units you can let me know and I'll try to help the maintainer out...

thomasp85 commented 5 years ago

I've added you as collaborator on this repo, so once you've accepted you should be able to merge, close, etc. As we are in very different timezones this may easy development a bit (and it's your package after all 🙂)... If you are happy with the changes I've made to Ops.unit() you can try out your new powers by merging this into #1

pmur002 commented 5 years ago

Thanks Thomas. I have tried out the merge. Please let me know if I mucked anything up.

pmur002 commented 5 years ago

I have sent a cautious query to CRAN about the 'ggplot2' hot-fix - will let you know if I hear anything.

thomasp85 commented 5 years ago

Ok - the merge seems fine... In general these PRs are only there to create patches so merging will be kept at a minimum. I've yet to set up something automated to keep the master synced with the svn version, so if you notice it lagging behind just let me know

pmur002 commented 5 years ago

Cool. Will do.

I see a ggplot2 3.1.1 on CRAN - is this the hotfix ? Will (re)commence my testing!

thomasp85 commented 5 years ago

Yes. It just got accepted 🎉

pmur002 commented 5 years ago

My testing shows all know CRAN package failures are fixed. But the CRAN ggplot2 check results page https://cran.r-project.org/web/checks/check_results_ggplot2.html still shows 3.1.0 being used on some platforms (e.g. r-devel on Windows) so I am checking when that will update before I try to commit again.

thomasp85 commented 5 years ago

That’s great news! The windows binaries usually take some time before being available and it sounds good to wait

pmur002 commented 5 years ago

Advice from CRAN is to wait several days. That is likely to put us outside the window for 3.6.0 unfortunately. On the bright side, Brian has offered to do a "non-live" CRAN test of the new unit patch, so I have sent him a patch to try (without the risk of causing massive public chaos).

thomasp85 commented 5 years ago

It would really be a shame to miss 3.6.0

Looking forward to hear what BDR finds

pmur002 commented 5 years ago

Brian's tests have picked up a problem with 'GGally' (at least). The essence of the problem is this ...

x <- unit(1:3, "in") + unit(.5, "npc")
x[[2]] <- unit(1, "npc")

... which allows me to assign a "simpleUnit" into the middle of a proper unit. This can happen because there is no [[<- method for units (nor is there a [[ method for units). Unfortunately, having diagnosed the problem, I am running out of time to fix it (today). Would you be able to have a go at a [[<-.unit method and a [[.unit method ?

pmur002 commented 5 years ago

Random thought thrown out as I leave my office ... How hard would it be to replace the "old unit detected" error with code that "promotes" an old unit to a new unit (maybe with a warning). This would end up as a bit of a vestigial piece of grid code, but might significantly enhance the transition (by not requiring people to reinstall 'ggplot2' and by allowing CRAN tests to pass sooner rather than later).

thomasp85 commented 5 years ago

Had it just been a single simple unit class I think it would be fairly easy, but promoting e.g. unit.arithmetic would require considerable work

thomasp85 commented 5 years ago

I’ll make a [[ and [[<- method today

pmur002 commented 5 years ago

Thanks. I'll take a look at that. In the meantime, Brian has completed the off-line CRAN testing and it has thrown up a further myriad of failures (at least partially due to a myriad of mis-uses of units). Sorry, but this is not going to make 3.6.0. There are too many things to fix and they are in too many different places (packages). Apologies for my slowness in getting on to this patch in the first place.

What is the best way to record the problems (I have at least 23) ? A github issue ? Or several separate issues ?

pmur002 commented 5 years ago

FYI, I am out of action Mon + Tues next week. Back on deck Wed.

thomasp85 commented 5 years ago

Yeah... I can see from the reverse dependency failures that this has to be postponed. On the flip side we'll now have a whole year to polish this 🙂

I'll make a single issue based on your mail where we can tick off problems as we solve them, or decide that package authors need to change their code. As the pressure of deadlines has now been lifted a bit I'll attend to some other urgent tasks before beginning to tackle these

pmur002 commented 5 years ago

Yep, I saw the same silver lining :) Thanks for setting up the github issue list.