sindresorhus / atom-editorconfig

Helps developers maintain consistent coding styles between different editors
https://atom.io/packages/editorconfig
MIT License
812 stars 80 forks source link

Later rules should override earlier ones #151

Closed jamesarosen closed 7 years ago

jamesarosen commented 7 years ago

Later rules for insert_final_newline aren't overriding earlier ones in the same file. According to https://github.com/editorconfig/editorconfig/issues/278, they should. Note that the spec states that later rules override earlier ones and not more specific rules override more general ones.

When I save foo.js, the configuration below correctly inserts a newline at the end. When I save foo.hbs, it incorrectly inserts a newline at the end.

Involved .editorconfig-files

root = true

[*]
end_of_line = lf
charset = utf-8
trim_trailing_whitespace = true
insert_final_newline = true
indent_style = space
indent_size = 2

[*.js]
indent_style = space
indent_size = 2

[*.hbs]
insert_final_newline = false
indent_style = space
indent_size = 2

[*.css]
indent_style = space
indent_size = 2

[*.html]
indent_style = space
indent_size = 2

[*.{diff,md}]
trim_trailing_whitespace = false

Directory structure

$ ls -am
., .., .editorconfig, foo.hbs, foo.js

Installed packages

Built-in Atom Packages (89)
├── atom-dark-syntax@0.27.0
├── atom-dark-ui@0.52.0
├── atom-light-syntax@0.28.0
├── atom-light-ui@0.45.0
├── base16-tomorrow-dark-theme@1.3.0
├── base16-tomorrow-light-theme@1.3.0
├── one-dark-ui@1.6.2
├── one-light-ui@1.6.2
├── one-dark-syntax@1.5.0
├── one-light-syntax@1.5.0
├── solarized-dark-syntax@1.0.5
├── solarized-light-syntax@1.0.5
├── about@1.7.0
├── archive-view@0.62.0
├── autocomplete-atom-api@0.10.0
├── autocomplete-css@0.13.1
├── autocomplete-html@0.7.2
├── autocomplete-plus@2.31.4
├── autocomplete-snippets@1.11.0
├── autoflow@0.27.0
├── autosave@0.23.1
├── background-tips@0.26.1
├── bookmarks@0.42.0
├── bracket-matcher@0.82.2
├── command-palette@0.39.0
├── deprecation-cop@0.54.1
├── dev-live-reload@0.47.0
├── encoding-selector@0.22.0
├── exception-reporting@0.40.0
├── find-and-replace@0.202.0
├── fuzzy-finder@1.4.0
├── git-diff@1.1.0
├── go-to-line@0.31.0
├── grammar-selector@0.48.2
├── image-view@0.60.0
├── incompatible-packages@0.26.1
├── keybinding-resolver@0.35.0
├── line-ending-selector@0.5.0
├── link@0.31.2
├── markdown-preview@0.158.8
├── metrics@1.1.1
├── notifications@0.65.1
├── open-on-github@1.2.1
├── package-generator@1.0.1
├── settings-view@0.243.1
├── snippets@1.0.3
├── spell-check@0.68.4
├── status-bar@1.4.1
├── styleguide@0.47.2
├── symbols-view@0.113.1
├── tabs@0.103.0
├── timecop@0.33.2
├── tree-view@0.210.0
├── update-package-dependencies@0.10.0
├── welcome@0.35.1
├── whitespace@0.35.0
├── wrap-guide@0.38.2
├── language-c@0.54.0
├── language-clojure@0.22.1
├── language-coffee-script@0.48.0
├── language-csharp@0.12.1
├── language-css@0.40.1
├── language-gfm@0.88.0
├── language-git@0.15.0
├── language-go@0.43.0
├── language-html@0.46.1
├── language-hyperlink@0.16.1
├── language-java@0.24.0
├── language-javascript@0.122.0
├── language-json@0.18.3
├── language-less@0.29.6
├── language-make@0.22.2
├── language-mustache@0.13.0
├── language-objective-c@0.15.1
├── language-perl@0.37.0
├── language-php@0.37.3
├── language-property-list@0.8.0
├── language-python@0.45.1
├── language-ruby@0.70.2
├── language-ruby-on-rails@0.25.1
├── language-sass@0.57.0
├── language-shellscript@0.23.0
├── language-source@0.9.0
├── language-sql@0.25.0
├── language-text@0.7.1
├── language-todo@0.29.1
├── language-toml@0.18.1
├── language-xml@0.34.12
└── language-yaml@0.27.1
Community Packages (15) /Users/me/.atom/packages
├── atom-material-syntax@0.4.6
├── atom-material-syntax-light@0.4.5
├── atom-material-ui@1.3.6
├── atom-ternjs@0.15.0
├── change-case@0.6.3
├── dash@1.7.0
├── editorconfig@2.0.4
├── ex-mode@0.13.1
├── file-icons@1.7.24
├── git-blame@0.4.11
├── linter@1.11.18
├── linter-jscs@4.1.0
├── linter-jshint@3.0.1
├── merge-conflicts@1.4.4
└── vim-mode@0.65.1
florianb commented 7 years ago

Thanks @jamesarosen - i think this is a bug but i fear it belongs to https://github.com/editorconfig/editorconfig-core-js since we're belonging on it. I would like to advice you opening your issue there.

Sorry that i couldn't help so far. 💝

jamesarosen commented 7 years ago

Moved to editorconfig/editorconfig-core-js#41

florianb commented 7 years ago

@jamesarosen thanks a lot & good luck. Let me know if you need help.

jamesarosen commented 7 years ago

I don't think this is an issue with editorconfig/editorconfig-core-js. At least as of the latest version, I get the following:

var editorconfig = require('editorconfig');

editorconfig.parse('foo.js').then(function onFulfilled(result) {
  console.log(result.insert_final_newline);
}); // true

editorconfig.parse('foo.hbs').then(function onFulfilled(result) {
  console.log(result.insert_final_newline);
}); // false
florianb commented 7 years ago

Thanks @jamesarosen - i will take a look.

florianb commented 7 years ago

Hi @jamesarosen - sorry for replying that late.

I spend now a fair hour to get behind what your problem might be.

At first, i can't reproduce the described behavior -- not within Atom, nor with the editorconfig-binary from the command line.

I also tried to dig into the editorconfig-core-js but, well i guess i'm too tired to get a quick insight. However, i have the suspicion that calling editorconfig.parse() with relative paths may not lead to any expected behavior. As far as i understood the implementation, absolute paths are needed at this point.

Without any additional information i can only guess. This behavior may probably be caused by the fact you didn't define a root = true and/or any symlinks are interfering the editorconfig-resolvement.

I would really like to know what this causes. So if you want do further investigations let me know.

dmytrokyrychuk commented 7 years ago

@jamesarosen

When I save foo.js, the configuration below correctly inserts a newline at the end. When I save foo.hbs, it incorrectly inserts a newline at the end.

I couldn't reproduce this behavior in with atom-editorconfig v2.2.0.

I don't think this is an issue with editorconfig/editorconfig-core-js. At least as of the latest version, I get the following:

var editorconfig = require('editorconfig');

editorconfig.parse('foo.js').then(function onFulfilled(result) {
  console.log(result.insert_final_newline);
}); // true

editorconfig.parse('foo.hbs').then(function onFulfilled(result) {
  console.log(result.insert_final_newline);
}); // false

Are you running this in node.js REPL or in Console tab of Developer Tools (within Atom)? Are the results above different from what you would see after you run EditorConfig: Show State from Command Palette?

@florianb

i have the suspicion that calling editorconfig.parse() with relative paths may not lead to any expected behavior. As far as i understood the implementation, absolute paths are needed at this point.

The current version of editorconfig-core (0.13.2) would pass the path through path.resolve() during editorcinfig.parse() (code), which would deal with . and .. in the path, and prepend current working directory if the path is not absolute. I don't think this is documented anywhere. Relative paths may work well when working directory is set appropriately, because of the details of the current implementation, but it is not guaranteed to stay this way. They advise to use absolute paths with the CLI tool (FAQ).

jamesarosen commented 7 years ago

@tundal45 @amyrlam could one of you provide any extra details here?

florianb commented 7 years ago

Thanks for the hint @orgkhnargh - i took that away from the cli and consequently overlooked path.resolves normalization step.

florianb commented 7 years ago

I close this issue due to missing response -- feel free to come back if you think this issue needs more attention.

Thank you for contribution! 💝

jamesarosen commented 7 years ago

I think the issue was having atom's whitespace package installed.