markmcintyre / brackets-jslint

MIT License
6 stars 5 forks source link

Do not emit errors for lines with only spaces #1

Closed Mark-Simulacrum closed 10 years ago

Mark-Simulacrum commented 10 years ago

Use code from https://github.com/adobe/brackets/blob/master/src/extensions/default/JSLint/main.js#L75. Using @TomMalbran's suggestion in adobe/brackets#8842.

markmcintyre commented 10 years ago

Hey, it's my first pull request! Thanks, @Mark-Simulacrum. I have a concern, though, in that adding this feature effectively changes the behaviour of JSLint. That is, the output from this extension would be different than if you were to copy/paste code directly at jslint.com. My feeling is that to suppress these errors, it's probably better to either use the fantastic "Strip Whitespace" extension by @MiguelCastillo, or enable the /*jslint white: true */ directive. What do you think?

Mark-Simulacrum commented 10 years ago

Hmm, I would agree - that is the case. However, it feels like either this should be a preference specified by the extension, or at least better documented in the README/Documentation for the extension.

Using /* jslint white: true */ works, but is slightly cumbersome and seems odd (especially on large projects), so I would go with one of the two options above - and I think the preference option would be the best in this case.

Perhaps @TomMalbran would have some ideas about this as well, since he suggested this addition to the extension.

TomMalbran commented 10 years ago

A preference would work fine. In my code I have white lines with spaces so that the cursor doesn't move around so much when moving up or down. So using the white jsliny pref is not an option for me. I could also get used to have empty lines, but I would have to fix lots of files.

Mark-Simulacrum commented 10 years ago

As @TomMalbran says, adding the JSLint directive is generally not an option for many projects/people. I'd be happy to attempt adding a preference for this feature, although not exactly sure where to start with that. (Although I'm fairly certain it'd be fairly easy to find in other extensions, so I should be able to do it.).

markmcintyre commented 10 years ago

Good points! Currently, the extension supports project-level preferences through the .brackets.json file, so you don't have to add jslint directives to every file in your project. Would adding white: true to those preferences work for your use-case, @TomMalbran?

If not, I'd be up for adding a preference to the extension. A checkable menu item, or even just another preference option in the .brackets.json file would do the trick. :)

Mark-Simulacrum commented 10 years ago

A checkable menu item seems like something that would be more than this feature really needs, since (as @markmcintyre said) you can just add white: true to the .brackets.json file.

markmcintyre commented 10 years ago

I tend to agree, since it keeps the extension simple and in line with the core JSLint functionality. At the very least, though, I can update the README.md file as @Mark-Simulacrum suggested, particularly since Brackets users are probably used to not having whitespace warnings reported.

TomMalbran commented 10 years ago

As I said, I can't add white: true as I want to get all the other errors that JSLint returns. You don't need to add a menu for the preference. Just adding it to the file is enough, and is pretty easy to do. Something like this in the code should work:

var PreferencesManager = brackets.getModule("preferences/PreferencesManager"),
    prefs              = PreferencesManager.getExtensionPrefs("jslint");
prefs.definePreference("lintWSLines", "boolean", true);

...

function lintFile(text, path) {
    if (!prefs.get("lintWSLines")) {
        text = text.replace(/^[ \t]+$/gm, "");
    }
}
markmcintyre commented 10 years ago

That works for me, @TomMalbran. :+1: Would you like to put that into a new pull request? Otherwise I can add this feature myself as soon as I get a chance. (Either way, we should also document this in the README.)

Mark-Simulacrum commented 10 years ago

@markmcintyre I'd be happy to put this into a new pull request.

markmcintyre commented 10 years ago

Sounds great. Thanks, @Mark-Simulacrum! I'm going to close this pull request now.

TomMalbran commented 10 years ago

I won't be able to try this until the weekend. So feel free to give it a try. I can check the PR later and help if required.