New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

no-mixed-spaces-and-tabs doesn't properly handle jsdoc tabs/spaces #1055

Closed
spocke opened this Issue Jul 8, 2014 · 7 comments

Comments

Projects
None yet
3 participants
@spocke

spocke commented Jul 8, 2014

test.js

(function() {
    "use strict";

    /**
     * x
     */
    return function() {
    };
})();

Command line:
eslint test.js

Result:
test.js
5:1 error Mixed spaces and tabs no-mixed-spaces-and-tabs
6:1 error Mixed spaces and tabs no-mixed-spaces-and-tabs

Should do an exception here since it's very common to have these kinds of mixed space/tabs comments.

@jrajav

This comment has been minimized.

Show comment
Hide comment
@jrajav

jrajav Jul 8, 2014

Contributor

Could you clarify what you mean by this having worked before? This rule was created a couple of weeks ago and only just released yesterday with 0.7.1.

It looks like this case probably wasn't accounted for - maybe we should discuss if this is worth a silent special case or if it should just fall under the smarttabs option.

Contributor

jrajav commented Jul 8, 2014

Could you clarify what you mean by this having worked before? This rule was created a couple of weeks ago and only just released yesterday with 0.7.1.

It looks like this case probably wasn't accounted for - maybe we should discuss if this is worth a silent special case or if it should just fall under the smarttabs option.

@spocke

This comment has been minimized.

Show comment
Hide comment
@spocke

spocke Jul 8, 2014

My mistake it worked before since then it was using the older version. So that didn't throw this error.

spocke commented Jul 8, 2014

My mistake it worked before since then it was using the older version. So that didn't throw this error.

@jrajav

This comment has been minimized.

Show comment
Hide comment
@jrajav

jrajav Jul 8, 2014

Contributor

So bcd5c02 works but 0792ad8 does not (the only two commits: https://github.com/eslint/eslint/commits/master/lib/rules/no-mixed-spaces-and-tabs.js)? Still very odd since the regex was not changed. Could you construct a minimal case that fails on one but passes on the other (with tabs and spaces marked to see)?

Contributor

jrajav commented Jul 8, 2014

So bcd5c02 works but 0792ad8 does not (the only two commits: https://github.com/eslint/eslint/commits/master/lib/rules/no-mixed-spaces-and-tabs.js)? Still very odd since the regex was not changed. Could you construct a minimal case that fails on one but passes on the other (with tabs and spaces marked to see)?

@spocke

This comment has been minimized.

Show comment
Hide comment
@spocke

spocke Jul 8, 2014

The above files are the minimal test case with steps to reproduce it. Not sure how I can provide more detailed instructions than this except that github converts mixed tabs/spaces to spaces. It breaks in 0.7.1 and works if I downgrade to 0.6.2 that doesn't support the rule.

spocke commented Jul 8, 2014

The above files are the minimal test case with steps to reproduce it. Not sure how I can provide more detailed instructions than this except that github converts mixed tabs/spaces to spaces. It breaks in 0.7.1 and works if I downgrade to 0.6.2 that doesn't support the rule.

@jrajav

This comment has been minimized.

Show comment
Hide comment
@jrajav

jrajav Jul 8, 2014

Contributor

Oh, I see. You're just saying that it worked before this rule was ever created (since it was turned on by default).

So to reiterate, the questions here are now: Should this be special cased even with smarttabs turned off? Should smarttabs be on by default? Should the rule be off by default?

Contributor

jrajav commented Jul 8, 2014

Oh, I see. You're just saying that it worked before this rule was ever created (since it was turned on by default).

So to reiterate, the questions here are now: Should this be special cased even with smarttabs turned off? Should smarttabs be on by default? Should the rule be off by default?

@spocke

This comment has been minimized.

Show comment
Hide comment
@spocke

spocke Jul 8, 2014

Yes, it started breaking when the new rule was applied but the rule it self isn't properly working with normal formatted jsdoc style block comments so it's a new bug not a regression.

spocke commented Jul 8, 2014

Yes, it started breaking when the new rule was applied but the rule it self isn't properly working with normal formatted jsdoc style block comments so it's a new bug not a regression.

@nzakas nzakas added bug labels Jul 8, 2014

@nzakas

This comment has been minimized.

Show comment
Hide comment
@nzakas

nzakas Jul 8, 2014

Member

Working on this.

Member

nzakas commented Jul 8, 2014

Working on this.

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