search

philipwalton / html-inspector

HTML Inspector is a code quality tool to help you and your team write better markup. It's written in JavaScript and runs in the browser, so testing your HTML has never been easier.
2.32k stars 145 forks source link

Support for CLI #14

Closed webpro closed 11 years ago

webpro commented 11 years ago

See #9.

philipwalton commented 11 years ago

Hey, nice work overall. There are a couple things I noticed and have questions about.

1) I'm seeing the errors logged twice. I looked at the two files in ./bin and didn't immediately see why, so I thought I'd just ask.

2) I think it would be great to, by default, check for the presence of an html-inspector-config.js file in the current directory and, if found, load that if nothing was passed via -c.

3) I don't have much experience with node CLI stuff, but I'm wondering if console.log is the best way to report the errors. If the point of the CLI is for other programs to be able to run the tests and read the results, shouldn't we use STDOUT or STDERR, or is console.log aliased to that? Thoughts?

4) I noticed that you're listening for messages logged to the console in PhantomJS via page.onConsoleMessage. I think there's probably a better way since the page being loaded may be logging to the console itself for legitimate reasons. Probably the best bet is to override the onComplete config option with something else.

gotdibbs commented 11 years ago

With # 4 might I suggest tying into onAlert instead of onConsoleMessage? You'd have to inject a bit of bridge code to accomplish that, but it is exactly what I'm doing in the grunt task I made (and it works well) so you could borrow the bridge from that if you wanted.

webpro commented 11 years ago
  1. I'm not seeing them twice. Are you sure? E.g. when I do htmlinspector http://google.com I'm seeing some errors multiple times, but that's correct. How can I reproduce your case?
  2. Me too.
  3. Since console.log writes to STDOUT, I think it's OK.
  4. Good point, and thanks @gotdibbs! I borrowed your bridge :-)

Will commit some stuff after this.

philipwalton commented 11 years ago

1) Gah, I'm an idiot. I was running the script on my local test page, which (obviously) was already making a call to HTMLInspector.inspect(). Non-issue.

3) Cool. Like I said, I'm not very familiar with these things, so whatever you think is good is fine by me.

philipwalton commented 11 years ago

Thanks @webpro I merged this in to master. There are a few minor changes I may make in the future, but I figured it'd be best to get this in and let people use it so I can get some feedback.

Random question (please excuse my ignorance): your examples used the htmlinspector command, but are you actually able to use that command literally as-is? How is that found in your path, or did you manually add it? I saw that the bin property was added to package.json, but I'm not sure if that has anything to do with anything.

Anyway, thanks again for your work.

webpro commented 11 years ago

Yes, by all means, release early :-) Feel free to ping me in any issues/RFC's people may have.

And yes, the globally available htmlinspector command is the beauty of npm install -g. That was actually the purpose of this CLI exercition. With a global install, the project is installed in a global node_modules registry on your system, and npm adds a symlink htmlinspector (referencing bin/html-inspector.js) to the bin folder (/usr/local/bin in my case).

However, you need to submit your project to the npm registry for users to globally install this module. You can do that with npm publish from the project root. Let me know if you need any info.

Without a global install, it's still usable, but people would need to do e.g. node path/to/bin/html-inspector.

philipwalton commented 11 years ago

Ahh, that makes sense. I'll register it with NPM before I release the next version.

niksy commented 11 years ago

I didn’t see this package on npm registry, so I tried installing it manually and I get this error:

$ npm install -g https://github.com/philipwalton/html-inspector/archive/0.4.1.tar.gz
npm http GET https://github.com/philipwalton/html-inspector/archive/0.4.1.tar.gz
npm http 200 https://github.com/philipwalton/html-inspector/archive/0.4.1.tar.gz
npm http GET https://registry.npmjs.org/commander
npm http 304 https://registry.npmjs.org/commander
npm http GET https://registry.npmjs.org/keypress
npm http 304 https://registry.npmjs.org/keypress
npm ERR! Error: ENOENT, chmod '/usr/local/share/npm/lib/node_modules/html-inspector/bin/html-inspector'
npm ERR! If you need help, you may report this log at:
npm ERR!     <http://github.com/isaacs/npm/issues>
npm ERR! or email it to:
npm ERR!     <npm-@googlegroups.com>

I suppose this is related to npm not finding the correct file in bin folder—currently html-inspector has .js extension.

philipwalton commented 11 years ago

Thanks. I'll fix this in the next release and add it to NPM.