Filenames in output should be relative to where csslint was run from

[jonathanj]

Running "csshint styles/nav/main.css" from ~/foo produces "main.css" as the filename. Apart from making it harder to figure exactly which "main.css" for a human, it makes it impossible for something parsing the output to determine exactly which file this refers to; using Vim's quickfix window and csslint as the compiler, for example.

[nzakas]

Can you give an example of the behavior you don't like? We have several different output formats and I'm not sure which you're referring to.

[jonathanj]

For compact and text formatters (lint-xml seems to include the absolute path) only the filename is specified, even if the filename is not in the same directory that csslint is being run from. Assume you have a hierarchy like:

foo
|-- bar
|   `-- quux.css
`-- quux.css

csslint's (version 0.5 from npm) output looks like:

$ csslint --format=compact foo

quux.css: line 1, col 6, Heading (h1) should not be qualified.

quux.css: line 1, col 1, Rule is empty.

Ideally the filename should be relative to where you ran csslint from (e.g. foo/bar/quux.css and foo/quux.css) or an absolute path so that it's easier for both humans and computers to determine the location of the file in question.

[eriwen]

I agree that we should change this, however, it seems that including just the filename was very intentional (since before formatters were introduced). I'd be curious to know the logic behind that decision before we make changes.

It would be trivial to include the absolute path, relative path may be a bit more difficult but will have to dig deeper.

Please advise.

[nzakas]

In general, we can't change what's present in a given output format unless it's a serious bug. There are tools that rely on the existing formats for generating their output. For example, the CSS Lint Textmate bundles relies on the default format to generate their output.

I'm not opposed to adding a new format that has absolute paths. I'm not sure if it should be a derivative of the default or compact formats though. Thoughts?

[eriwen]

Seems silly to have a separate output format for just this, but is it? (let's forget about the whole csslint-xml vs lint-xml thing for a moment) It's the most backward-compatible way to implement this AFAIK. Also seems like the time to do small compatibility-breaking things is before 1.0.

Do many large projects have this problem of duplicate file names?
Do we know if the CSS Lint TMBundle would break if we introduced this change? Be nice if it was robust enough to handle it.

TL;DR - If stability of other apps is the number one concern, new output format it is.

[nzakas]

Despite the pre-1.0 version number, I don't believe in breaking already-existing implementations based on one person's request for a change.

I'm also not convinced that creating a clone of compact format with the full path is useful, so I'm wondering if a new format that's a derivative of the default would actually be better. The default format already outputs the full path name at the start of the linting for that file.

@jonathanj - in an ideal world, what would your output format look like to make it easier to integrate with vim? Assume you can have anything you want.

[jonathanj]

@nzakas ideally a relative path and as few blank lines as possible (currently there are several blank lines between different message types.)

For what it's worth, node-jshint's output is ideal, it's incredibly straight-forward to parse with vim without any ambiguities.

[eriwen]

@nzakas defer to you how we name it and what changes (relative vs. absolute path and whitespace) you want, though I'm sure you could crank out the new format as quickly as you would answer those questions.

I'm happy to do it this weekend if you let me know.

[nzakas]

@jonathanj - would you mind pasting the output of that? I don't have a node installation handy at the moment.

[jonathanj]

@nzakas sure, again a directory hiearchy like this:

foo
|-- bar
|   `-- quux.js
`-- quux.js

Running node-jshint gives output like:

$ jshint foo
foo/bar/quux.js: line 2, col 3, ['foo'] is better written in dot notation.
foo/bar/quux.js: line 2, col 14, Missing semicolon.
foo/quux.js: line 1, col 9, It is not necessary to initialize 'foo' to 'undefined'.
foo/quux.js: line 1, col 20, Missing semicolon.

4 errors

This is incredibly close to the "compact" formatter for csslint although there are some notable differences: Filenames are relative to where I ran the tool from, there are no leading newlines and no newlines between files.

I'm not sure what other tools rely on the "compact" format or how they parse the output but it seems unlikely that changing the compact format to match node-jshint's output would break them. To me it seems superfluous to create a new formatter for these small changes, obviously that's your decision to make.

[nzakas]

Okay, I'm sold on changing the compact format to match JSHint's output format. @eriwen - do you want to take a stab at that?

[eriwen]

@nzakas sure thing.

[eriwen]

@nzakas I thought quite awhile about how to properly implement relative paths output. The problem is that we would have to pass a base directory (preferred) or a relative file path all the way from where we process command-line arguments (fixFilenames()) through processFiles > processFile > formatResults in all formatters. Seems like a very significant change. I can't figure out a way to get the relative path without having environment-specific logic somewhere where we don't want it.

@johathanj Absoute paths would be much easier to get than relative paths here, would that be sufficient?

[jonathanj]

I would prefer relative paths.

I'm not sure I follow your reply to @nzakas. You mention that you don't want environment-specific logic somewhere that doesn't need it, but wouldn't the case of transforming an absolute path to a relative path be a thing that needs environment-specific logic?

To me it would seem ideal to pass a FilePath object around (with methods such as relpath, filename, etc.) instead of an opaque string. There is already logic in each formatter to cut a path up and extract the filename, with fallback cases for Windows paths, etc. etc.

(FWIW, processFile invokes formatResults with what appears to be an extraneous argument.)

[eriwen]

@jonathanj I really like the idea of passing a FilePath Object around instead of a string with the absolute path. It'll still be a decent-size change because we have to change a few APIs. What I meant by "environment" before was runtime like NodeJS or Rhino. I'll tackle this change soon.

I noticed that we pass the formatter as an extra argument, and I have no idea why. Hopefully whomever wrote those lines can explain it to us.

[nzakas]

I'm not particularly thrilled with passing an object around for this. Even though the formatting methods are currently only used by the CLI, that doesn't mean it will always be the case, and I'd like to make sure anything can easily make use of these APIs - so changing an argument to an object that needs multiple properties to work isn't a great idea. Basically, the formatters should have no idea what environment they're running in.

That said, it seems like the easiest thing to do would be to add an optional currentDirectory argument after filename for formatResults. That way, the formatter itself can choose how to deal with that information (or not).

[eriwen]

How about this, then: add an options {Object} argument to the end of the appropriate signatures and add a "baseDirectory" option, which would also allow us to add a "quiet" option for issue #170.

[nzakas]

Sounds good.

[nzakas]

After thinking about this and poking around in the code a bit, I have a little different opinion than I did before.

First, I think the only reason that the full file path is passed into the formatter is because the default text format uses it. No other format really needs it and I'm not even sure the default format really needs it.

Given that, I'd propose the following changes:

1. Pass the relative path of the file into formatters.
2. Add another argument for options, one of which could be fullPath.
3. Ensure compact format uses the relative path for all messages (right now it uses filename for errors/warnings and full path for "Lint free" messages)

Thoughts?

[eriwen]

The XML formats use absolute paths, but the Lint XMLs do not depend on them. Not certain about Checkstyle, but my educated guess is also that an absolute path is not necessary.

I'm not very partial to either relative or absolute paths, but if I were forced to vote, it would be for absolute paths simply because I sometimes find myself having the same repository in multiple places. Honestly, though, I'd be happy as long as it's not just the filename.

You can see what I did for adding options with relative paths in my commits 53fc7a..59bdb8 in my repository. I won't submit a pull request until we're decided, but at least you can get an idea what the code might look like. Luckily, the buik of the work is done either way.

Any other votes?

[nzakas]

My thought process went like this: in general, it tends to be more common to use relative paths than absolute paths. By providing an absolute path and a base directory, we'd be forcing format developers to resolve the path themselves, which doesn't seem necessary or practical. Whereas providing the relative path as the default argument and the absolute path as an additional piece of info on the options object means that no path resolution is required.

Does that make sense?

[eriwen]

Very much so. You'll see all the work I had to do to get the relative path in the compact formatter in my repo, though we could probably refactor that stuff into a utility method.

I agree with your suggestion and will submit a pull request as soon as I can. In the meantime, feel free to pull in request #185 if you like and I'll merge my changes with that cleanly.

[nzakas]

Why don't you go ahead and make the changes, then submit a pull request for the whole thing.

[nzakas]

Fixed with last pull request.