Wednesday, April 22, 2009

$:.unshift(File.dirname(__FILE__))

This shit is evil. People are undecided as to how evil, but evil it is.



$: is a way-too-Perl-y special variable alias for $LOAD_PATH. You could change it like this:

$LOAD_PATH.unshift(File.dirname(__FILE__))

The upside: you're now doing something more comprehensible. The downside: that "something" is editing an all-caps global. That doesn't feel wrong to you?

This code pops the local directory name (relative, not absolute) onto the load path. Then you can require filenames as relative paths. This is not such a good idea.

Say you've got a gem which provides an interface to the Twitter API, and this gem uses this convention. This is just an example; I'm not thinking of any real code out there on the githubs. Say you're enabling your web app with some Twitter-related functionality. Maybe you've got files like lib/twitter.rb or app/models/twitter.rb. The problem comes not with this idiom, but from what it makes possible. It enables you to do this:

require 'twitter'

In this particular example, that shit breaks all over the fucking place.

Yay! I saved myself the effort of typing an absolute path and/or code which would generate an absolute path! And in the process, I guaranteed myself unexpected load bugs. The code attempts to load lib/twitter or app/models/twitter and you get MethodNotFoundErrors and similar problems. This idiom is only a good idea if your filenames are like the Highlander and there can be only one.



You can make it a little nicer:

$LOAD_PATH.unshift(File.expand_path(File.dirname(__FILE__)))

But you're still looking at a mess, and if you're going to go to all that trouble, you're better off doing this:

require File.expand_path(File.dirname(__FILE__)) + "twitter"

Of course the nicer thing is to do this:

class File
def self.here(string)
expand_path(dirname(__FILE__)) + string
end
end

require File.here "twitter"


I think in reality File is a module, but you get the idea. You see File.expand_path(File.dirname(__FILE__)) all over the place, and it's ugly as sin. You see $:.unshift(File.dirname(__FILE__)) now and then, and it's not just ugly, but also vulnerable to annoying breakage. Any time you're faced with a choice between ugly shit and ugly shit that doesn't even work, the only sane response is to say "fuck no" and rewrite the rules so you're not faced with that kind of dilemma again.

15 comments:

  1. Holy. Freaking. Crap.

    Dude, get out of my head! Today at work I thought "I'm really sick of this File.expand_path(File.join(File.dirname(__FILE__),...)) crap. What I really want is something like 'require from_here "file_name"'.

    Spooky.

    ReplyDelete
  2. There are some upsides. If you are dealing with semi-canonical 'twitter' gem, it is nice to be able to preload you own require 'twitter' before a gem the requires that as a dependency.

    So maybe it is just a little hubris, you are saying this is "the twitter.rb" That gets at what I was also saying about being relative from the name of the gem onward, such as the idiom for requiring from the common library:

    require 'cgi/session'

    This works if you are dealing with a case where there is a defacto or real standard, such as for uniquely named gems (say hpricot or something).

    All I know is I have seen cases where it is cleaner and avoids loading issues better if some things are purely relative.

    ReplyDelete
  3. @chalain: If that's enough psychic shit to freak you out, be glad you're not me. You would have gone insane by now. That shit happens to me so often it doesn't even make me blink. Look in the archives of this site and find the story about the screenplay I wrote on 9/07/01 with images of spaceships crashing into the earth, smashing buildings, and throwing up huge clouds of dust. It mentioned anthrax being used as a weapon, too, and had a lot of content about the first war with Iraq. I finished it four days before 9/11.

    ReplyDelete
  4. @timocratic: well here's the thing. I've never seen this thing and not had it be a problem - but I've never gone digging through the loading code for a gem that wasn't working. maybe in a browsing, idle curiousity sort of way, but I never take a serious look at a gem's loading code unless I'm having some kind of problem with loading stuff. so I probably have seen this approach not causing problems, but I don't know about it.

    but the simpler argument is, this is way too much thought for what should be a simple operation. I said this in IRC right after you signed off, Phil is right, Java actually had a good idea here. Loading objects in Java uses syntax that ignores the filesystem and puts all its attention on the object space. I'm sure there's some nightmarish configuration that masks the goodness of the idea, but the long and the short of it is that loading files into memory really only deserves a few moments of thinking at most and $: runs a really high risk of going over budget.

    It's like if your stapler cost $3 in most cases, and $350 every so often. So you've got a department full of staplers, most cost $3, two of them cost $350 for no good reason. Paying that much money for a stapler is ridiculous, and the inconsistency makes it impossible to run a business. For the same reason, something like loading a file should always be cheap in terms of dev thinking. You need some simple shit that won't break. I'm all in favor of "magic" and so-called "metaprogramming," but that much idiosyncrasy for such a simple purpose is really not a wise idea.

    ReplyDelete
  5. There are two bugs in your code, alas. The first one is that you'll want to insert a "/" between the result of expand_path() and string.

    The second is more significant: the value of __FILE__ will always be the path to the file that *defines* File.here, rather than the file that calls it.

    So, if this method is defined in, say, "/tmp/lib/file_here.rb", and you have two files, one being "/tmp/my_lib.rb", which defines your twitter code or whatever, and the second being "/tmp/my_app.rb", which is where you want to require your library:

    require "lib/file_here.rb"
    puts File.here("my_lib") # ignoring the slash problem
    require File.here("my_lib")

    then the output is "/tmp/lib/my_lib", not "/tmp/my_lib", along with a LoadError.

    So, basically, this will only work if the File.here method is defined by a file in the same directory as the other files you want to require using it.

    ReplyDelete
  6. .... that is, unless you plan on defining File.here at the top of every file that plans on using it, of course.

    ReplyDelete
  7. And you have to anyways, if you are going to use it before any require... That is the real problem with the require mess, you need to fix it before you could require a file that would fix it.

    Giles, I was actually thinking last night about something like autoload, but for non-lazy requiring. Sorta auto_require, or a require that instead of determining wether to load/reload the file based on the filepath string passed into require, did so based on the constant specified, autoload style. Constants have to be unique, within namespacing anyways, so it'd be nice if that were the actual determinate. Hell it could even do rails style expecting the constant to match the file name. Wait. don't they have a method for that... ;)

    ReplyDelete
  8. Of course, we could just all switch to 1.9. ;)

    ReplyDelete
  9. @lazyatom: the specifics of my code are irrelevant. The first sentence after the code example acknowledges that it's pseudocode.

    ReplyDelete
  10. @timocratic - I'm skeptical about 1.9, and I don't have time to port every gem and Rails app I work on or with. I think that autoload thing sounds fun but I don't think it'd be much use for Rake tasks. (Thor tasks, tho, quite possibly.)

    ReplyDelete
  11. God, do not get me started. Every time I see someone chewing up ARGV the red mist descends...

    ReplyDelete
  12. But I don't want necessarily a require of my vendored lib to be counted as a different file than some other version, and thus both loaded, due to having different absolute paths.

    require 'canonical_lib/subset' is correct often.

    That's why I am thinking some form of

    auto_require :ConstantName, 'whatever fucking path you

    ReplyDelete
  13. yeah actually reloading isn't really what you want to do nine times out of ten. I wouldn't call it auto_require but maybe something that treats class loading as something nice and simple. I'm definitely going to look at using Dave's stuff today but I can see the arg for a better class-loader too. and actually it would kinda be nice if we adopted some (dare I say it) Java-esque naming standard for lib/gem/plugin name-spacing.

    ReplyDelete
  14. Ah! I always had a feeling that something where goint wrong with the sentence $LOAD_PATH.unshift(File.dirname(__FILE__)) but I couldn't specify what exactly. Now things are cristal clear. Thanks Giles.

    ReplyDelete

Note: Only a member of this blog may post a comment.