Cast two-digit version numbers to character

[peterhurford]

Resolves https://github.com/robertzk/lockbox/issues/119

[peterhurford]

@russellpierce

[kirillseva]

foo.yml

name: RSQLite
version: 2.0
load: false

in R:

kk <- yaml::yaml.load_file('foo.yml')
kk$version %>% as.character
kk
# $name
# [1] "RSQLite"
#
# $version
# [1] 2
#
# $load
# [1] FALSE

kk$version %>% as.character
# [1] "2"

[peterhurford]

@kirillseva I see what you mean about how 1.0 gets turned into "1"...

[peterhurford]

@kirillseva Looks like a custom float handler solves the issue!

[peterhurford]

Please test this more :)

[kirillseva]

I like the tests and the improved modularity of this pull-request, but I feel like introducing custom yaml loading rules and casting everything as characters, including booleans, is overengineering the problem and makes debugging future issues more complicated.

Especially when the solution to the problem is using a couple of quotes here and there

[russellpierce]

I like the idea that this would make the lockfile just work and hide the complexity from the user.

What if someone decides to version their package just by integers with no decimal, would this still work?

What gets me with 119 is that it is just detecting the existing version that fails and triggers a reinstall. Lockbox then goes on to load things just fine.

If a solution like this isn't accepted, could we find a middle ground with an informative warning when the version specified risks running into this issue?

[kirillseva]

Or we can update the readme so that all examples show version listed in quotes and avoid overengineering On Sun, Aug 20, 2017 at 3:11 PM Russell S. Pierce notifications@github.com wrote:

I like the idea that this would make the lockfile just work and hide the complexity from the user.

What if someone decides to version their package just by integers with no decimal, would this still work?

What gets me with 119 is that it is just detecting the existing version that fails and triggers a reinstall. Lockbox then goes on to load things just fine.

If a solution like this isn't accepted, could we find a middle ground with an informative warning when the version specified risks running into this issue?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/robertzk/lockbox/pull/120#issuecomment-323605227, or mute the thread https://github.com/notifications/unsubscribe-auth/AEO9uLsUM4WYCakVzQ_D_UCPWC46kAY1ks5saITOgaJpZM4O8biA .

[peterhurford]

I like the idea that this would make the lockfile just work and hide the complexity from the user.

is overengineering the problem and makes debugging future issues more complicated

It's definitely a trade-off. I'm open for a vote on this.

~

I like the tests and the improved modularity of this pull-request

Either way, I do think we should keep that. I can resubmit the PR if desired.

~

casting everything as characters, including booleans

Good catch. I fixed that so that booleans stay as booleans. I added test coverage for that too.

And thanks for catching the embarrassing sprintf("%.1f", 1.0) fix. ...This is why I made quickcheck for R. ...I added some more tests around that.

~

What if someone decides to version their package just by integers with no decimal, would this still work?

Yes.

[russellpierce]

Vote: Yes

The PR is well compartmentalized and tested. For now it solves a problem that affects the usability of the package. I grant that the PR represents extra complexity. However, there is nothing to say we can't revert that complexity down the line if it becomes a problem. In the near term I don't see much reason to shy away from it.

At the very least, let's get the passing tests merged - then we can leave updating the docs or adding a warning to another PR.

[kirillseva]

Oh tests are here to stay, I'm not letting them go :) but I am of an opinion that we do not need any extra magical typecasting under the hood On Tue, Aug 22, 2017 at 2:45 PM Russell S. Pierce notifications@github.com wrote:

Vote: Yes

The PR is well compartmentalized and tested. For now it solves a problem that affects the usability of the package. I grant that the PR represents extra complexity. However, there is nothing to say we can't revert that complexity down the line if it becomes a problem. In the near term I don't see much reason to shy away from it.

At the very least, let's get the passing tests merged - then we can leave updating the docs or adding a warning to another PR.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/robertzk/lockbox/pull/120#issuecomment-324131760, or mute the thread https://github.com/notifications/unsubscribe-auth/AEO9uFAtUdw7WpJw1kkyGlnvJxJ1-A6dks5say_vgaJpZM4O8biA .

[kirillseva]

Here's how I would deal with this situation if it were kirillseva/lockbox :)

1. Merge https://github.com/robertzk/lockbox/pull/121
2. Pluck tests out of this PR and merge them
3. Do not do any hidden logic in between yaml <--> R reading as that could potentially cause a lot of confusion at the most uncomfortable time

But that's just like my opinion, we're all obviously waiting on @robertzk's thoughts on this matter

[peterhurford]

@kirillseva I'm happy to let @robertzk decide. I can assist with making a new PR with the tests + refactor if @robertzk decides to go off of #121 instead of #120.

[peterhurford]

Either @robertzk merged https://github.com/robertzk/lockbox/pull/121 without thinking of this PR or he has voted against the custom character parsing. @robertzk can you weigh in explicitly?

[peterhurford]

@robertzk ping

[robertzk]

@peterhurford Ping!

[robertzk]

@peterhurford It looks like the float handler wasn't doing as much as expected. (Putting a stop in the is.numeric(x) branch showed it doesn't execute at all during tests.) I'll merge when green.

[peterhurford]

@robertzk Thanks for finishing this up!

[peterhurford]

@robertzk @kirillseva Should we revert https://github.com/robertzk/lockbox/pull/121 in light of this PR merge?

[robertzk]

@peterhurford Good point!