Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

sugar.rb causes 5.kilobytes and 5.bytes to return the same value #61

Open
camertron opened this Issue · 15 comments

6 participants

Cameron Dutro Aman Gupta Eric Lindvall Tom Preston-Werner Jeff Dutil Chris Corbyn
Cameron Dutro

If you're (for whatever reason) loading God alongside a Rails app, 5.kilobytes and 5.bytes will return the same value of 5 because God's sugar.rb uses kilobytes as the smallest size granularity while ActiveSupport uses bytes. Why not use the same conventions as Rails? This isn't a big deal because there's really no reason to include the God gem in your gemfile (as I mistakenly did), but the question remains - why not go as small as bytes?

Aman Gupta
Collaborator

This should be fixed. Can you provide a pull request?

Cameron Dutro

I did a gem install of god a few weeks ago and got version 0.11.0. Unpacking it revealed the lowest granularity to be kilobytes. Do I have an outdated version of the gem? I can work up a patch for this and submit a pull request if you like.

Aman Gupta
Collaborator

https://github.com/mojombo/god/blob/master/lib/god/sugar.rb still only has kilobytes.

This will also require changing any code in god that is expecting a kilobytes value to convert it from bytes first.

Cameron Dutro

I'll see what I can do.

Eric Lindvall
Collaborator

Cross posting from the pull request:

I have trouble seeing how we can ever merge this.

We have no way of knowing the number of people who aren't using the sugar and are just expecting that anything that accepts an integer is expecting to assign things in kilobytes.

Eric Lindvall
Collaborator

@camertron: Can I ask why you're loading god into your rails app?

Is this because it's being auto-loaded because it's in your Gemfile? If so, would this solve it?

gem 'god', :require => false
Cameron Dutro

As I wrote earlier, I'm not actually loading god into my rails app. At first I was, but I discovered the error of my ways and removed it from my gemfile. I'm not currently experiencing this issue, but I posted it here to see what you all thought. The only reason I "fixed" this and submitted a pull request was because @tmm1 asked me to. Merging this in would require that all users of the god gem are made aware of the fact that the paradigm has shifted from thinking of everything in kilobytes to bytes, which you can decide to enforce or not enforce. It does still seem a little strange to me to expect the smallest size granularity to be kB, but it's more of a philosophical concern at this point than one that will cause problems. It's up to you!

Tom Preston-Werner
Owner

In practice this would screw over too many people. There's no way that everyone would know about the change before upgrading and I'm not prepared to cause the total implosion of thousands of servers worldwide. I made the decision when I originally wrote the code to use kB as the smallest unit because there was no reason anyone would ever want to use bytes in the memory condition. I agree it was the wrong decision, but it's too late now. But I don't think it matters that much. As long as god and rails don't mix, everyone is fine.

Tom Preston-Werner mojombo closed this
Jeff Dutil

Could a sort of deprecation notice be implemented, and a changelog in the readme or separate file like announce.txt? Not technically a deprecation, but some sort of warning to alert developers when upgrading god.

It seems to me like it should eventually be fixed even though it doesn't need to be right away. After all god is versioned, and developers can expect some incompatibility fixes being required when upgrading for major version changes like going to v1.0.0.

Tom Preston-Werner
Owner

It could be done at a major version barrier, but I'd insist it be done in a way that prevented god from starting if you used a bare integer in one of those conditions. We could make a Byte class that would be returned by 5.kilobytes or whatever and check that in the relevant conditions. Then at least upgraders would have to fix their configs instead of being bitten by an insidious and invisible change.

Chris Corbyn

Using a notation like 1.kilobyte isn't generally necessary and, as with too many other gems IMHO (particular those that patch the Symbol class), you just end up battling it out with other gem developers as to who is most correct. I'm going to bet ActiveSupport were defining Numeric#kilobyte before god was.

What about a notation like [1, :kilobyte], [350, :megabytes]., or even just a String: "350MB"... the most "fundamental" behaviour being to supply a numeric type, for the number of kilobytes.

Can you at least make it so that sugar.rb must be explicitly required, if developers want those core extensions? At least that way it's opt-in, not forced-in.

Eric Lindvall
Collaborator

Related: #17

Chris Corbyn

I'd also argue that you can just let people include ActiveSupport, if they want numeric helpers. A non-implicitly included sugar.rb will likely fix this, however.

Eric Lindvall
Collaborator

Fixed in 0.13.1.

Eric Lindvall eric reopened this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.