search

jejacks0n / teaspoon

Teaspoon: Javascript test runner for Rails. Use Selenium, BrowserStack, or PhantomJS.
1.43k stars 243 forks source link

Allow customizable asset path normalization. #164

Closed rwjblue closed 10 years ago

rwjblue commented 10 years ago

We are using a custom extension which passes through a pre-processor (in our case converting ES6 module semantics with AMD output), and need to instruct Teaspoon to use the correct extension on the served files.

bcardarella commented 10 years ago

:+1:

joefiorini commented 10 years ago

:thumbsup:

jejacks0n commented 10 years ago

Nice, thanks! So, I just started on 0.8 (might become 1.0) and I'm changing a lot about configuration and trying to simplify some of the existing configuration. Since this effects that, it won't be able to merge correctly.

I'll integrate this into my new branch (that I haven't pushed yet), and after more review I'll merge this into 0.7. Just checking to see if you want it merged on 0.7 and have it changed for 0.8 or if you'd rather have it go in once with a consistent API for 0.8?

rwjblue commented 10 years ago

As it is now, we have to use a fork to use a custom extension. If you don't mind (and there are no issues to address after further review) I'd like to get it in on master (current 0.7 series) for now, and then update/upgrade to the new API once its ready. I'd prefer to point folks to the main repo vs using a fork (and forgetting to swap after 0.8/1.0 is out)...

@jejacks0n - Ultimately it is totally up to you, we can make it work either way.

jejacks0n commented 10 years ago

The implementation looks solid. I was out earlier, so I wasn't able to look at it until now.

Here's some thoughts, and think about them for a bit while I do the same. I totally understand the need for this, and I think the approach is fine (though, I would change the lambda to a proc -- not the stabby kind, just cause that's how I roll. ;-P), and I think this implementation is good as well.

In the past year I've merged a lot of PRs that add to the configuration, and while this is fine, I've noticed it getting cluttered and dirty -- which is one of the primary things I'm trying to improve on now, so understand my hesitation when there's "yet another configuration added".

I'm wondering if you think this will be a common thing? I haven't seen it up until this point, but it doesn't seem wrong to do it. I guess my point, is that if I'd structured that method nicer, would you have been ok with overriding it from the initializer by reopening the class? You can see the things that I'm trying to balance, and so I'm curious what your thoughts are. Let's say I broke off the filename.gsub calls into a single line method so it can be easily overridden.

Trying to figure out if there's a less "easy" way, but also a less "configuration-y" way. I'm pretty on the fence, but if there's a way to do it without more configuration I'd be happy.. still trying to keep the barrier to entry low with teaspoon.

rwjblue commented 10 years ago

I completely agree with the less is more (configuration wise) direction that you seem to be going in. The reason that I implemented this as I have is mostly due to the way the existing configuration has been done. I would personally prefer a more API based approach that would allow my to create a class that has all of the required methods (or inherit from the default), and only override the specific methods that I needed to tweak.

The issue in this case was that the method that needed to be changed actually had more than one function (or at least combined two smaller functions): 1) Lookup the requested object in the asset pipeline, and 2) normalize the filename returned. If these parts were split out into separate functions, I would have been perfectly happy to monkey-patch to override the individual part that I needed to modify (frankly we may still monkey-patch in our gem depending on the timing of a release with these changes).

@jejacks0n - I am unsure if this clarifies the issue or makes it muddier, but I certainly hope that it is more of the former. I would definitely be happy to help hack out a public API that could be used for custom runners/configurations etc. Feel free to reach out (my contact email is in my profile) if you'd like.

jejacks0n commented 10 years ago

Awesome, yes, and that's a great suggestion. I think I may play around with that idea, so you can use a custom Suite class if you want, which would give you pretty much full access to whatever you wanted. I'll think on your ideas, and see what I can come up with.

I won't be releasing the gem, but I'll merge this and revise for 0.8.