-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
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
v3.6 ignores "smart-tabs" rule and notifies of indent errors #7248
Comments
Note that I've just clamped the eslint dependency to 3.5. Please keep this in mind when trying to reproduce the bug, you might need to |
We did recently add functionality to support reporting of mixed spaces and tabs in the indent rule. This was done as a generalization of solving the issue of indent failing to fix tabs to spaces or vice versa (i.e., not a mix, just the wrong character outright). Given that we have Marked "evaluating" for now because I can't confirm on my mobile, but I am 95% sure it's reproducible. @IvanSanchez Could you help us out by providing a code example? You've only listed configuration and an error message. Thanks! |
Confirmed in online demo with this example: /* eslint-env es6 */
/* eslint indent:["error", 4], no-mixed-spaces-and-tabs:["error", "smart-tabs"] */
const foo = "bar",
bar = "baz"; // N.B. One tab, two spaces Hope it's suitable. |
@platinumazure I'm a bit confused by the example; shouldn't that report an error if the expected indent is 4 spaces? |
no-mixed-spaces-and-tabs flags mixture of space and tabs, except in certain circumstances. In "smart-tabs" mode, it allows you to use a mixture if it's for the purpose of lining up a variable name. One could make an argument that perhaps the mixed tab/space responsibility does belong to indent, but historically they have been separate rules. I'm still hoping for a better example from @IvanSanchez at some point, since I don't know if my example is very good or not. |
I think I understand the general idea, but looking at your example: /* eslint-env es6 */
/* eslint indent:["error", 4], no-mixed-spaces-and-tabs:["error", "smart-tabs"] */
const foo = "bar",
bar = "baz"; // N.B. One tab, two spaces The expected indent for that line is 4 spaces, so even ignoring the tabs, |
Well, as I said, I'm making do and inferring based on insufficient information. I guess the correct thing to do here is to have eslintbot get involved... |
Hi @IvanSanchez, thanks for the issue. It looks like there's not enough information for us to know how to help you. If you're reporting a bug, please be sure to include:
|
OK, I'm not used to your workflow so I wasn't fully aware that you expected a minimal example from me. Sorry about that. I copy-pasted some of our production code and made this small reproducible example:
It seems that tabs/spaces get lost in markdown formatting, so I've put the code in a gist. Note the spaces and tabs in the variables declaration:
Using eslint v3.5.0 (or any previous version), there are no linting errors. This is the expected behaviour. Using eslint 3.6.1, I get the following output:
|
It looks like the only problem here is that the smart tabs option isn't being respected. If we can fix that, we should be good to go. |
I'll take a look. |
I'm not sure it's that simple-- smart-tabs is an option in |
Ah yes, looked at this too quickly, you're right. I'll look at reverting that for now and we can revisit whether or not there should be two rules at a later point. |
@platinumazure it looks like this change came in as a result of an issue you opened: #4274. Can you comment on the effect if we revert this commit? (8b3fc32#diff-f7ae198ea24182d178b8f3376e61b6ed) |
@nzakas Let me know if this makes sense:
So if the commit is reverted, the problems described in 1 and 2 above will reappear. IMO, the correct solution is for indent to only report if there are no mixtures of tabs and spaces (i.e., either only spaces are used when tabs should be, or vice versa, or if the correct indent character but wrong number of characters is used). Obviously, if we are considering a patch release to fix this, a full revert may be simpler and the correct solution can go in the next minor. Hope this helps, let me know if I can clarify anything. |
A simple fix would be to have a special case in edit: Alternatively, we could just have |
@not-an-aardvark Agreed, that's the correct solution. The question is whether we want to put that in right before an emergency patch release, rather than just revert the commit so we're at least in a known state. |
We inadvertently introduced a breaking change here, and we need to fix that quickly. If someone can make the correct fix, that's fine, but if no one has the time to do that today, then we should revert the commit to do a patch release. We can always add back the changes later with the correct fix. This is also a good reminder to make sure each PR solves only one problem so that reverts are clean. |
I made a PR to fix the issue at #7266.
Fair enough, will keep that in mind for future PRs. |
I'm a maintainer of Leaflet, and we use an eslint config with
smart-tabs
, like:eslint 3.0 through 3.5 work just fine. However, after an upgrade to eslint 3.6, we've got error messages like the following:
This can be reproduced by checking out the Leaflet repo, then running
npm install
and thennpm test
. Note that runningnpm install eslint@3.5
thennpm test
will work as expected.We don't use any transpiler in the Leaflet toolchain; full eslint config is available on the Leaflet repo; and the bug is reproducible on the environments of more than one developer.
Looking at eslint's changelog, I'll guess that this is caused by #7076 (ping @not-an-aardvark)
The text was updated successfully, but these errors were encountered: