search

robertzk / lockbox

Bundler-style dependency management for R
MIT License
49 stars 11 forks source link

Two digit version numbers with trailing zeros are mishandled. #119

Closed russellpierce closed 7 years ago

russellpierce commented 7 years ago

Consider the case of ...

  -
    name: RSQLite
    version: 2.0
    load: false

This YAML will trigger the error Error: invalid version specification ‘2’. If you provide a trailing .0 for version 2.0.0 the package will install, but it will also install each time the lockfile is specified. The package itself only has a Major and minor (no patch #). So, 2.0 is probably the right way to specify the version.

russellpierce commented 7 years ago

yaml is converting 2.0 to a (numeric) 2, and base::package_version() is what emits the error given a numeric 2 as an input.

russellpierce commented 7 years ago

We could special case versions that are doubles that can be expressed as integers using something like package_version(sprintf(2, fmt = "%.1f")) but that seems ugly. Any better ideas / am I looking at this issue in the wrong way?

kirillseva commented 7 years ago

@drknexus try this:

-
    name: RSQLite
    version: "2.0"
    load: false
russellpierce commented 7 years ago

🤦‍♂️ Well... that works perfectly. Thank you. I suspect this is still an 'issue', but suddenly a far less important one. :)

kirillseva commented 7 years ago

It's not an issue, it's yaml spec :( and the way yaml package implements it On Fri, Aug 18, 2017 at 2:03 AM Russell S. Pierce notifications@github.com wrote:

🤦‍♂️ Well... that works perfectly. Thank you. I suspect this is still an 'issue', but suddenly a far less important one. :)

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/robertzk/lockbox/issues/119#issuecomment-323267499, or mute the thread https://github.com/notifications/unsubscribe-auth/AEO9uO5xQJDIhs0ymsYF-PTc1hJnytiHks5sZSlAgaJpZM4O7GOg .

russellpierce commented 7 years ago

YAML (seemingly appropriately) considers 2.0 numeric (aka double). Where problems came up was that base::package_version() expects a well formatted a character input... but since R is type promiscuous it will happily take numerics as well (until it doesn't). It does seem like lockbox (or base) could be doing something more kind here. At least in the docs we could format the versions as strings as opposed to numerics? I'll sleep on it, but if y'all want to close that is fine with me too. Thanks for the quick rescue!

kirillseva commented 7 years ago

It parses 2.0 as numeric, and we wish it parsed it as a string. The way to tell the parser that we expect a string is to use quotes. It's actually a fairly common yaml gotcha

Feel free to close the issue of this answer satisfies you (im currently replying on the phone, don't have a close button available) On Fri, Aug 18, 2017 at 2:12 AM Russell S. Pierce notifications@github.com wrote:

YAML (seemingly appropriately) considers 2.0 numeric (aka double). Where problems came up was that base::package_version() expects a well formatted a character input... but since R is type promiscuous it will happily take numerics as well (until it doesn't). It does seem like lockbox (or base) could be doing something more kind here. At least in the docs we could format the versions as strings as opposed to numerics? I'll sleep on it, but if y'all want to close that is fine with me too. Thanks for the quick rescue!

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/robertzk/lockbox/issues/119#issuecomment-323268645, or mute the thread https://github.com/notifications/unsubscribe-auth/AEO9uA-1XY3aczNcXN7qkEWDmzN-oSueks5sZStAgaJpZM4O7GOg .

peterhurford commented 7 years ago

@kirillseva @robertzk We could amend the yaml reader to change all the inputs to strings.

kirillseva commented 7 years ago

or update the readme with this example

russellpierce commented 7 years ago

I do prefer @peterhurford's solution as it is the kind of thing that seems like we can make 'just work' rather than adding mental load.

kirillseva commented 7 years ago

I think because of

> as.character(as.numeric("2.0"))
[1] "2"

this would not be a successful attempt, and making a PR to yaml package would break the yaml spec. Unfortunately, this one is on users :'(

Unless we wan to shy away from yaml and move to another format for describing the lockfile, which is very much possible. At the end of the day, that's what bundler does (bundler's lockfile is written in ruby)

peterhurford commented 7 years ago

@kirillseva Be more imaginative :) https://github.com/robertzk/lockbox/pull/120