seattlerb / debride

Analyze code for potentially uncalled / dead methods, now with auto-removal.
https://www.zenspider.com/projects/debride.html
720 stars 19 forks source link

File extensions handled by plugins are coupled to gem name #25

Closed phiggins closed 8 years ago

phiggins commented 8 years ago

At $dayjob we've swapped out the original haml gem for an alternate implementation, hamlit: https://github.com/k0kubun/hamlit . Now we are in the delightful situation where the original haml gem can't properly parse our templates, so my plugin debride_haml just raises an exception when I try to run debride on the project. I made a new plugin to use hamlit for template parsing instead (https://github.com/phiggins/debride-hamlit), but it currently does not work because debride expects that the file name lib/debride_hamlit.rb means my plugin parses files with the file extension of .hamlit.

I can think of a few ways to fix this but wanted to get your input:

  1. Rename lib/debride_hamlit.rb in my debride-hamlit plugin to just lib/debride_haml.rb. This might work but will probably cause weirdness with things like bundler expecting the name of the gem and the name of the file to require to be the same.
  2. Use some fancier introspection to determine the file extensions to examine based on the methods debride plugins define. I hacked out a quick version of this already to get me up and running: https://gist.github.com/phiggins/3052bfce1e843e2e6e38f01bf90e6f15
  3. Have debride plugins register the file extensions they parse by adding them to the fileextensions array, or something. I kind of like this idea because of the explicitness, but it would also cause it's own problems because of the need to keep the list of extensions and the `process*` methods in sync.
zenspider commented 8 years ago

So, to be painfully clear... you're using a new gem that isn't backwards compatible but using the same file extension?

I prefer option 1. I don't care about bundler oddities esp around autoloading as I think it is an abomination meant to slow down all projects everywhere. You can get around that by having both files in your plugin.

Option 2 isn't a bad way to go either, but it won't work well as the SexpProcessor method naming convention is process_<type>.

phiggins commented 8 years ago

So, to be painfully clear... you're using a new gem that isn't backwards compatible but using the same file extension?

Yes, that's what I thought the problem was.

As I mentioned in chat, I was getting some confusing exceptions that led me to believe they were incompatible, but I'm not sure now. After further investigation there was one line of code that caused debride to raise an exception, and since it contains the new "&." operator it makes me suspicious that something in debride and friends isn't handling that properly. I'll try to make a minimal reproduction (that does not involve haml, if possible) and open a new issue if necessary.

phiggins commented 8 years ago

I prefer option 1. I don't care about bundler oddities esp around autoloading as I think it is an abomination meant to slow down all projects everywhere. You can get around that by having both files in your plugin.

Option 2 isn't a bad way to go either, but it won't work well as the SexpProcessor method naming convention is process_.

It annoys me mildly when the gem name and the name of the file you require to use it aren't the same, but given the edge-case nature of this problem I'd understand if this was the way you wanted to handle it.

The patch I included at least got my new plugin up and running, but it is a bit squirrelly and could probably cause problems of its own.

phiggins commented 8 years ago

This was a total red herring. Sorry for the noise.

zenspider commented 8 years ago

In what way?

phiggins commented 8 years ago

I was confused by https://github.com/seattlerb/ruby_parser/issues/214 and https://github.com/seattlerb/debride/issues/24 and thought the problem was incompatibility between the two haml gems.