Git Product home page Git Product logo

Comments (6)

ttilley avatar ttilley commented on July 25, 2024

The load path is altered in three places... The rakefile, the spec helper, and the main fssm.rb file. The only lines that matter for this topic are in fssm.rb:

dir = File.dirname(__FILE__)
$LOAD_PATH.unshift dir unless $LOAD_PATH.include?(dir)

Anywhere else that the load path is touched, I've deliberately pre-expanded the path. It makes me wonder why I didn't do so here. -_-

I'm going to, at the least, change that to:

dir = File.expand_path(File.dirname(__FILE__))
$LOAD_PATH.unshift dir unless $LOAD_PATH.include?(dir)

If you load FSSM via rubygems then those two lines should have absolutely no effect whatsoever. If you use some other path management system then, in theory, the same should be true. However, FSSM is thrown into a vendor directory of some kind more often than not, and I don't want to unintentionally break other software. Those lines ensure that THIS version of FSSM is first if it was specifically loaded from a location not on the load path.

from fssm.

ttilley avatar ttilley commented on July 25, 2024

...good catch though. jeez. how did I get that right where it didn't matter, and not where it did? orz

I'll keep this issue open for a bit in case you have additional concerns, but those lines are specifically there for use outside the context of rubygems.

from fssm.

ttilley avatar ttilley commented on July 25, 2024

A new version of FSSM has been released addressing this, as well as a deprecation warning when using 1.9.3.

from fssm.

luislavena avatar luislavena commented on July 25, 2024

The load path is altered in three places... The rakefile, the spec helper, and the main fssm.rb file

You don't need to touch $LOAD_PATH inside specs, mostly because RSpec (and most of the testing frameworks) already add it when invoking the child process (see the command line that RSpec::Task builds for you for example)

If you load FSSM via rubygems then those two lines should have absolutely no effect whatsoever. If you use some other path management system then, in theory, the same should be true.

That is partially true.

While File.expand_path will normalize the path and avoid it be duplicated in $LOAD_PATH, it will still cause a performance penalty of path expansion that can be avoided (since RubyGems already added it to the load path)

So: there is a performance effect on doing something that is already performed by the packaging tool.

In the case of RubyGems, which allows multiple versions be installed but doesn't allow multiple versions be activated at the same time, ensuring the path gets modified is pointless.

In the case of other package managers provided by the OS, they don't allow multiple versions, so is pointless alter the $LOAD_PATH with the assumption we need to ensure the latest.

After all, you're already requiring the right file, that means the right path is already there.

In the case of vendoring... well, is complicated and shouldn't be your job play with the $LOAD_PATH when a 3rd party is vendoring your library.

Please see the anti-patterns in relation to using RubyGems:

http://guides.rubygems.org/patterns/#loading-code

I understand your hesitation on release a newer version without this hack/approach, just wanted to share the rationale behind my request which was not a criticism on your code but instead a bad practice that can be corrected.

On a sidenote, on Windows, File.expand_path hits the filesystem, which means the more expand_path is performed the slower it works.

When loading multiple gems the abuse of multiple expand_path results in performance degradation.

from fssm.

ttilley avatar ttilley commented on July 25, 2024

::shrug:: FSSM isn't usually loaded multiple times, or loaded at all in scenarios where performance degradation of a require matters (like compass used in rails debug mode where stuff gets reloaded per request, but the request is the context for re-checking instead of a filesystem event so FSSM doesn't even get loaded). Since it hasn't mattered, not much thought went into it.

Note that it is possible to have FSSM installed as a gem and also have a gem that prefers its own vendored (not via bundler, but via having a 'vendor/fssm') version of FSSM... Though the two libraries that were the whole purpose of writing FSSM, compass and sass, now prefer a system version of FSSM over their own copies (and I obviously prefer that route). Also note that for a decent chunk of FSSM's life, it existed primarily as something vendored by compass and most people never installed the gem. One of the goals was to not require an additional dependency to be able to use those libraries and utilities standalone. Come to think of it, I don't think either of them actually have a gem dependency on FSSM even now (EDIT: compass does, sass does not).

If it matters enough for someone to say something, it matters. Though I wonder if anyone is depending, currently, on being able to simply load 'vendor/fssm/fssm.rb' and then having those later requires with 'fssm/blah' still work.

I understand your hesitation on release a newer version without this hack/approach, just wanted to share the rationale behind my request which was not a criticism on your code but instead a bad practice that can be corrected.

Meh, I appreciate criticism of code. It tends to result in better code, and means that someone is actually using my code. ;)
I do realize it's a bad practice, though the realization of it being a bad idea (TM) came after it was written and made use of. Compass was switched to prefer a system version of FSSM quite some time ago (EDIT: and currently has a gem dependency like it should).

Do you have thoughts on gracefully ditching those lines? It's quite probably safe at this point.

from fssm.

ttilley avatar ttilley commented on July 25, 2024

the Listen gem under the Guard org will replace this, and be written from scratch with best practices in mind.

from fssm.

Related Issues (20)

Recommend Projects

  • React photo React

    A declarative, efficient, and flexible JavaScript library for building user interfaces.

  • Vue.js photo Vue.js

    🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.

  • Typescript photo Typescript

    TypeScript is a superset of JavaScript that compiles to clean JavaScript output.

  • TensorFlow photo TensorFlow

    An Open Source Machine Learning Framework for Everyone

  • Django photo Django

    The Web framework for perfectionists with deadlines.

  • D3 photo D3

    Bring data to life with SVG, Canvas and HTML. 📊📈🎉

Recommend Topics

  • javascript

    JavaScript (JS) is a lightweight interpreted programming language with first-class functions.

  • web

    Some thing interesting about web. New door for the world.

  • server

    A server is a program made to process requests and deliver data to clients.

  • Machine learning

    Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.

  • Game

    Some thing interesting about game, make everyone happy.

Recommend Org

  • Facebook photo Facebook

    We are working to build community through open source technology. NB: members must have two-factor auth.

  • Microsoft photo Microsoft

    Open source projects and samples from Microsoft.

  • Google photo Google

    Google ❤️ Open Source for everyone.

  • D3 photo D3

    Data-Driven Documents codes.