dominikh / filesize

filesize is a small ruby class for handling filesizes with both the SI and binary prefixes, allowing conversion from any size to any other size.
MIT License
81 stars 16 forks source link

Make class more coder-friendly #2

Closed mvastola closed 10 years ago

mvastola commented 10 years ago

I made minor changes to some existing code, but primarily what I did was add functionality that adds instance methods to the Filesize and Numeric classes when the Gem is loaded. This allows filesize calculations to be done much more easily and intuitively.

See example.rb for a demo of the added functionality and how it can be invoked.

Let me know if there are any questions/suggestions.

dominikh commented 10 years ago

Thanks for your patch, but I don't agree with half of it on principal. Monkey-patching existing classes, especially those from the standard library, is very bad form and should be avoided. As such I disagree with adding anything to the Numeric class.

I'm undecided about adding to* methods on instances of Filesize, but am leaning towards "no" simply because they seem somewhat out of place. Filesize is more meant as a library to deal with user input than adding new "primitives" for dealing with file sizes in Ruby code itself.

I have added some line comments for the sake of pointing out potential issues, but you don't need to fix them in the scope of this PR because, as I said, I don't feel inclined to merge it. But they might be nice to fix in your own code base if you decide to keep those changes in your own fork, or just monkey-patching this library (which still is evil ;))

mvastola commented 10 years ago

Oops. I'm on Ruby 2.0.. didn't check that String#chars returns an Enumerator instead of an Array (though I'd argue Ruby should have known to cast it).

Anyways, I doubt I'll be able to convince you here, and this isn't the proper forum for an in-depth debate (though I'm totally up for a more in-depth discussion about my philosophy on this if you would like), but:

I think the argument that "Monkey-patching is evil" is severely undermined by the fact that it is an ability fairly unique to Ruby and considerable effort has been made by the Ruby development team to not only keep the feature, but expand its capabilities (i.e. the addition of Module#prepend in 2.0.0).

As for a library vs. primitives, why should those be mutually exclusive? I think what I was going for with the ext/* dir (which I admittedly didn't fully implement) was the ability for the developer to choose whether or not they want to monkey-patch.

To me, one of my favorite things about Ruby is how clean the code can be. I saw/see this as a really useful Gem, but it seems almost anti-Ruby to have to put Filesize.from('1 MB') when you can have 1.MB instead.

Mike

dominikh commented 10 years ago

not only keep the feature, but expand its capabilities

Ruby 2.0 also added refinements to counter the dangers of monkey-patching, so that argument works in both directions.

just within the standard libraries overwriting Ruby's core

Green and orange apples. While slightly problematic, it is a lot easier for Ruby to be aware of its own optional additions. It is unlikely to step on its own toes, hopefully.

monkey-patching is used in Rails

Saying that Rails is doing something is not an argument for something. Besides, Rails itself is a framework/application, not a single library. If you create your own ecosystem, monkey-patching (by that framework or application) is far more tolerable than if you're just a tiny knob that shouldn't affect the behaviour of the rest of the system.

In fact, your megabytes example helps to demonstrate the problem: For the sake of argument, assume that megabytes and your proposed changes add the same methods to the same class, with slightly differing behaviour (such as, obviously, returning different types.). Next, assume that two libraries A and B use filesize and megabytes respectively. There'll be a conflict. And if the dependency on either of the two is a transitive one, deeply hidden in a chain of requires, it'll be rather hard to debug.

As for a library vs. primitives, why should those be mutually exclusive

I find it important for a library to be aware of its scope and purpose. The scope of this library is to have a programmatic interface to parsing and doing math with file sizes, with the primary focus on handling user input (in the form of text) as well as file system access (where you will be dealing with programmatically read byte numbers). Neither directly requires your methods.

One benefit of this approach is allowing straightforward implementations with a minimal amount of complexity. This encourages composition. It would be very easily possible for you to write a library that depends on filesize and adds the methods you desire to Numeric and Filesize (ignoring my concerns about monkey-patching for a second). At the same time, someone who is only concerned with the things that I made the primary focus will not have to deal with the state of his entire system changing.

it seems almost anti-Ruby to have to put Filesize.from('1 MB') when you can have 1.MB instead.

One can argue about what's Ruby and what is not, but if we accept that "1.MB" is idiomatic Ruby, then that's only true if you actually wish to encode a lot of file sizes directly in your code as if they were numbers. And while that might be true for your use case, it still falls outside of the scope of this library, which doesn't intend to extent the language itself.

You're right that you will not be able to convince me, and I am sorry that this library doesn't fit your requirements in all aspects, but I hope you understand my reasons.

dominikh commented 10 years ago

(Oh and I very much disagree with the way the "mathn" library works.)