sindresorhus / editorconfig-sublime

Sublime Text plugin for EditorConfig - Helps developers maintain consistent coding styles between different editors
MIT License
1.77k stars 108 forks source link

Indentation settings ignored on ST3 #46

Closed andig closed 9 years ago

andig commented 9 years ago

I have this rule in my .editorconfig:

[{composer.json}]
indent_style = space
indent_size = 2

Yet, when saving composer.json indentation is always converted back to tabs.

treyhunner commented 9 years ago

I believe (per the glob matching rules used by EditorConfig), a single {...} group is treated as if the { and } were literal.

Removing the { and } characters should fix the issue.

andig commented 9 years ago

Mhm. If true it doesn't fulfill the EC spec, but I'll test.

Update: Working if brackets removed. I believe this did work with brackets though in ST2, plus brackets are EC spec.

treyhunner commented 9 years ago

Unfortunately we don't have a formal grammar spec, so the core library test cases are effectively the spec.

The test cases dictate that {single} matches {single} and not single. See this two-line test and the corresponding test .editorconfig rule.

All of the core libraries pass that test suite and I believe we have supported brace usage that way since the beginning.

sindresorhus commented 9 years ago

@treyhunner Can you at least make that clearer on the website/docs? Not the first time I've seen this misconception ;)

andig commented 9 years ago

Unfortunately we don't have a formal grammar spec

I was referring to http://editorconfig.org which I thought was leading:

# Matches the exact files either package.json or .travis.yml
[{package.json,.travis.yml}]

My conclusion was that [{package.json}] constitutes a list with a single element and should therefore match package.json.

I'm fine with the resolution (much appreciated) but feel very much that other users will have the same problems...

sindresorhus commented 9 years ago

My conclusion was that [{package.json}] constitutes a list with a single element and should therefore match package.json.

I think that is much saner too and how all other globbing solutions works.

treyhunner commented 9 years ago

I also think that makes more sense, mostly because I can't see {...} being used much in filename matching. I have a feeling this was an implementation detail of one or more of our original glob matching libraries.

@xuhdev do you remember why we originally implemented it this way?

I wouldn't be opposed to making a breaking change for this because I doubt this feature/anti-feature would be missed.

xuhdev commented 9 years ago

@treyhunner I think it was just a slippery "doesn't make sense" and then it worked that way. I highly doubt this case was actually used though, so maybe breaking the compatibility can just make it less confusing and work better for automatic EditorConfig generators. I would say we need to bump a major version number to break this compatibility.

How about making a survey (such as a surveymonkey), to make sure that no one is using this feature?

andig commented 9 years ago

How about making a survey (such as a surveymonkey), to make sure that no one is using this feature?

Meanwhile, would it make sense to explicitly add

[composer.json]

as valid example to http://editorconfig.org?

I did not use this as I was- this is an additional question I'm wondering about- not sure about precedence. I was assuming { ... } is recognized as more specific than *.json.

Is there a precedence for the editor config rules? Order of declaration? Specificality of the globs (similar to CSS rules)?

sindresorhus commented 9 years ago

How about making a survey (such as a surveymonkey), to make sure that no one is using this feature?

I really doubt you'll find out that way. I've never seen it used like that so I think it will be easier to just break and make it what people assume now, rather than later.

sindresorhus commented 9 years ago

Also why aren't we just using https://github.com/isaacs/node-glob ?

treyhunner commented 9 years ago

I also don't think a survey would be very helpful. I think we should break it and version bump. Maybe we should look through the tests for other nonsensical edge cases and change any others that might be worth breaking during the version bump.

@sindresorhus we are using a fork of node-glob for the JS core library I believe because a few features had to be modified to match the format of the other glob matchers used in the C, Python, and Java libraries. I think the difference in ** was the biggest one.

xuhdev commented 9 years ago

I would agree to inspect some other edge cases and then decide whether we need a survey, just in case.