An alternate implementation #45

Closed
jgable opened this Issue Apr 17, 2013 · 9 comments

Comments

Projects
None yet
3 participants

jgable commented Apr 17, 2013

Why?

The existing grunt-contrib-jshint task at the time of this writing is hard to change and has limited unit tests. Rather than work the required large changes into your existing codebase I've decided to make grunt-jshint-bfs.

Significant Differences

Class Based

The units of work have been isolated into different classes rather than a single exposed init function. I believe this makes them easier to test and easier to expose as an API to other consumers.

More Unit Tests

I've tried to make sure the critical parts of the code have test coverage, whereas it seems that unit testing has fallen by the wayside for the contrib task.

Explicit jshint options

I've changed the options structure to be more explicit about what the jshint options are in an effor to separate them from the task specific options. I think this eliminates a lot of complicated option deleting code that is in the grunt-contrib-jshint codebase.

Asynchronous

I, personally, do not think that file reading should be done synchronously via the grunt.file.read function. Although my limited testing hasn't revealed any definitive improvements, I believe each file should be linted asynchronously for performance.

Caches Successful Runs

If we've previously run jshint on a file, it should be cached so that we do not need to lint it again. This is done through a combination of temp file based caching where a hash of the contents, the jshint options and the globals determine whether or not we need to lint the file again. This is on by default, but can be controlled via an option.

Reporter Architecture

While there is an existing Pull Request open (#34) to implement reporters, I am not happy with the hard linking of the reporter files and the hacky overriding of stdout necessary to make them work together nicely. I've ported over the existing xml and checkstyle reporters, and have plans to write a base legacy reporter class that will help others who have existing reporters built port them to be used with this task.

Misc. Style stuff

You guys are 2 space tab kinda guys, I'm a 4 space tab kinda guy. You guys are not fans of white space, I'm a big fan of it. Evidently you are also against single var statements for multiple assignment. I have no problem converting to your style if you want.

Where do we go from here?

I'd love to hear some feedback and have a dialog about the points I've raised above. I've never understood the init function based architecture in use and I'm curious if I might be missing something.

I'd be happy to talk about starting up a new branch with this code and merging it in over time. I'm sensitive to the options structure change and understand that it might be something that a minor version would be incremented for.

As a final option, if all my code just makes you want to vomit in your mouth a little bit, I could just publish this as a separate task and check back in from time to time looking wistfully at your code and thinking of the oh so sweet merging we could have done together.

Owner

shama commented Apr 18, 2013

The main problem with implementing features into your plugin is your effectively forking jshint. It isn't fair to the authors of jshint for us to add or modify their features. Any grunt jshint plugin should just provide access to the existing jshint features and if more is needed then those features should be implemented into jshint.

Which is why I think #35 is a better way forward and this plugin should try to do less and not more.

jgable commented Apr 18, 2013

Seems like that is not that big of an implementation change for me to switch over to. My goal at this time was to replicate the existing functionality and isolate code for testing.

Any comments on the init(..) function based architecture over a class based one? Asynchronous linting? Caching of successfully linted files?

Owner

shama commented Apr 18, 2013

The caching sounds interesting. How/where does it cache between runs? That might be better to discuss implementing upstream though, check out: gruntjs/grunt#694

I think we'd rather stick with init(grunt). I can see how a class architecture would be useful for organization on big projects. I'd rather just split up big modules into smaller modules though. Plus this plugin, atm, isn't meant to be an API consumed via require() so I don't see the value.

Re: async, Grunt is inherently synchronous so the performance gain would likely be very minimal. Let me know if I'm wrong and we get a big perf boost linting async though.

Re: tests, better test coverage is always good. I think reducing the amount of test needing code here and relying on jshint's tests would be ideal.

jgable commented Apr 18, 2013

The caching is done by getting a combination hash of the contents of the file, the jshint options (stringified) and the globals passed to jshint.lint. If the file is successfully linted we create a file in a os.tmpDir() based directory with that hash as the name and nothing in the file. On subsequent runs, we check for the existence of that file and can skip it.

The caching is also based on the version of the module, so updating the module will create a new cached file set.

The gist of the implementation can be found in jsHintFile and jsHintCache.js.

It's funny that you mention your API not being intended to be consumed by other people. Because of the way my plugin is architected you could essentially require my plugin and use that jsHintCache module for yourself here.

The problem I find with the current init method architecture is that your code is hard to test because it's hidden in that init function, which in turn is causing people to not want to write tests. And, as new features are added, that init function is going to have to be refactored to be more modular.

Re: async, on small sets of files the implementations are nearly identical in speed with a slight edge to your module here; I tested by using jquery mobile (around 75 files). I haven't had to time to put together a more involved test but I imagine that certain scenarios where there are a couple large files and several small files you would probably see an improvement. I'll get back to you when I write some better tests and have more concrete data.

I think your strategy of reducing code and keeping your implementation on top of that minimal is short sighted and doesn't leverage your position as a build system plugin. Last I checked, this module was downloaded ~45000 times this month. Imagine they use it to validate their files and it takes 2 seconds every time for an average of 5 times a day and a total of 10 seconds of their life. 10 seconds * 45000 is 125 hours a day that this module runs. If we can do optimizations (by implementing functionality of our own on top of jshint, which is what a build process is for) with our plugin that reduce that time by half that saves like 62 hours worldwide. In my mind that 62 hours of added productivity is spent saving puppies and small infants from trees and fires. I'm sure there is someone out there that gives medals for that type of shit, if not, I will totally send you one.

Owner

cowboy commented Apr 18, 2013

I'd recommend creating your plugin as a lib, maybe called jshint-cached and then create a very simple plugin around that as a Grunt-specific abstraction. Grunt plugins are generally intended to be lightweight abstractions around a core lib, so we recommend against making them any more complex than necessary.

jgable commented Apr 18, 2013

I figured I would have to make this as a separate plugin, but I thought I'd reach out to see how you felt about merging.

It's disappointing that we will not be able to save more puppies and babies together because of your massive install base.

If you want to take a shot at implementing the cache based approach in your own code you can find it in a fork I made for that purpose. It includes some tests, but will probably need to be refactored if you implement the switch to jshint.cli.

Owner

shama commented Apr 18, 2013

I'm all for optimizations as long as they actually are optimizations. Plus there are currently enough puppies and babies in the world. So I'm not too worried. :) Thanks for your time Jacob!

@shama shama closed this Apr 18, 2013

@jgable jgable referenced this issue in TryGhost/Ghost Oct 15, 2013

Closed

Command Line Tool #1006

jgable commented Nov 14, 2013

In retrospect, I'd like to apologize for the audacity of this issue and thank you for handling it in the professional way you did. I learned a little bit about "Hi guys, let's change all the things" PR's yesterday, and I definitely could have handled it better by taking your example here. Cheers 🍺 .

Owner

cowboy commented Nov 14, 2013

Cheers! 🍻

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment