Make WordPress Core

Opened 10 years ago

Closed 10 years ago

#28494 closed defect (bug) (fixed)

Multiple strings in .editorconfig section names should be wrapped in curly brackets

Reported by: netweb's profile netweb Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: 4.0 Priority: normal
Severity: normal Version: 4.0
Component: Build/Test Tools Keywords: has-patch commit
Focuses: Cc:

Description

via @treyhunner EditorConfig co-creator in #bbPress2614

The following sections are invalid:

  • [*.json,*.yml]
  • [*.txt,wp-config-sample.php]

They should instead be:

  • [{*.json,*.yml}]
  • [{*.txt,wp-config-sample.php}]

Square brackets for the section and curly braces to denote the either-or conditions in the shell glob.

See http://editorconfig.org/#file-format-details

Attachments (1)

28494.patch (392 bytes) - added by netweb 10 years ago.

Download all attachments as: .zip

Change History (9)

@netweb
10 years ago

#1 @netweb
10 years ago

  • Keywords has-patch added

#2 @jorbin
10 years ago

The curly syntax was added in 0.11.0 which came out in March. We originally added the .editorconfig in February, so I worry that this change will break editorconfig for developers. We should figure out what happens with various editors using editorconfig with the older version of editorconfigcore before making this change.

#3 follow-up: @treyhunner
10 years ago

Yes the curly brace syntax was added in 0.11.0 which was released in March 2013.

The current syntax is simply wrong, so any change is an improvement. The only kind of file [*.json,*.yml] will match is something like "word.json,anotherword.yml".

If you really don't want to use the curly brace notation you can always expand each glob into separate sections like jQuery still does in theirs: https://github.com/jquery/jquery/blob/949dfe52bc228efe2d0cb2d696b822ef2a5723d1/.editorconfig

Keep in mind that the curly brace notation is very common now and jQuery was one of the first projects to adopt the notation which is why they still use the old notation.

So in summary: I vote for the curly brace notation, but manually expanding into separate sections would work just fine as well. I'll just be glad to see the notation fixed to something parseable.

#4 in reply to: ↑ 3 ; follow-up: @netweb
10 years ago

Replying to treyhunner:

Thanks for the feedback Trey,

The current syntax is simply wrong, so any change is an improvement. The only kind of file [*.json,*.yml] will match is something like "word.json,anotherword.yml".

It wasn't wrong when we initially added .editorconfig to WordPress in February ;)

If we patch it using {curly braces}:

  • IDE/Text Editors with an up to date implementation of .editorconfig 0.11.0 spec will match those files.
  • IDE/Text Editors without an up to date implementation will not match the files correctly.

Is there a list of common IDE/Text Editors support of the 0.11.0 .editorconfig spec? Has it been widely implemented?

If you really don't want to use the curly brace notation you can always expand each glob into separate sections like jQuery still does in theirs: https://github.com/jquery/jquery/blob/949dfe52bc228efe2d0cb2d696b822ef2a5723d1/.editorconfig

Keep in mind that the curly brace notation is very common now and jQuery was one of the first projects to adopt the notation which is why they still use the old notation.

This is an option.

So in summary: I vote for the curly brace notation, but manually expanding into separate sections would work just fine as well. I'll just be glad to see the notation fixed to something parseable.

I'm also in favour of this now, per the files affected listed below this is a tiny set of files affected by this change.


Note: Files affected by this change:

  • wp-config-sample.php
  • wp-cli.yml
  • .travis.yml
  • package.json
  • 17 only 3rd party library license.txt, COPYING.txt, README.txt and similar named .txt files

#5 in reply to: ↑ 4 ; follow-ups: @treyhunner
10 years ago

Replying to netweb:

Replying to treyhunner:

Thanks for the feedback Trey,

The current syntax is simply wrong, so any change is an improvement. The only kind of file [*.json,*.yml] will match is something like "word.json,anotherword.yml".

It wasn't wrong when we initially added .editorconfig to WordPress in February ;)

Actually it was. Commas were not meant to denote separate files until we decided to add brace expansion in March. If those globs ever matched something without a ".json," in the middle of the filename and a ".yml" at the end, it was a bug in the parser being used. I'm fairly sure none of the parsers in February 2013 used commas to denote separate file globs.

If we patch it using {curly braces}:

  • IDE/Text Editors with an up to date implementation of .editorconfig 0.11.0 spec will match those files.
  • IDE/Text Editors without an up to date implementation will not match the files correctly.

Is there a list of common IDE/Text Editors support of the 0.11.0 .editorconfig spec? Has it been widely implemented?

This is specific to each editor plugin. I believe all of the plugins I know have been updated to 0.11 for some time (almost all since March 2013).

#6 in reply to: ↑ 5 @netweb
10 years ago

Replying to treyhunner:

Replying to netweb:

Replying to treyhunner:

Thanks for the feedback Trey,

The current syntax is simply wrong, so any change is an improvement. The only kind of file [*.json,*.yml] will match is something like "word.json,anotherword.yml".

It wasn't wrong when we initially added .editorconfig to WordPress in February ;)

Actually it was. Commas were not meant to denote separate files until we decided to add brace expansion in March. If those globs ever matched something without a ".json," in the middle of the filename and a ".yml" at the end, it was a bug in the parser being used. I'm fairly sure none of the parsers in February 2013 used commas to denote separate file globs.

Thanks, that pretty much decides things then, it was/is broken for everyone for the files and file types the 28494.patch​ applies to, so we aren't going to break any back compat with this patch.

If we patch it using {curly braces}:

  • IDE/Text Editors with an up to date implementation of .editorconfig 0.11.0 spec will match those files.
  • IDE/Text Editors without an up to date implementation will not match the files correctly.

Is there a list of common IDE/Text Editors support of the 0.11.0 .editorconfig spec? Has it been widely implemented?

This is specific to each editor plugin. I believe all of the plugins I know have been updated to 0.11 for some time (almost all since March 2013).

Awesome, thanks.

#7 in reply to: ↑ 5 @SergeyBiryukov
10 years ago

  • Keywords commit added
  • Milestone changed from Awaiting Review to 4.0

Replying to treyhunner:

It wasn't wrong when we initially added .editorconfig to WordPress in February ;)

Actually it was. Commas were not meant to denote separate files until we decided to add brace expansion in March. If those globs ever matched something without a ".json," in the middle of the filename and a ".yml" at the end, it was a bug in the parser being used. I'm fairly sure none of the parsers in February 2013 used commas to denote separate file globs.

Looks like this was broken in [28695].

#8 @SergeyBiryukov
10 years ago

  • Owner set to SergeyBiryukov
  • Resolution set to fixed
  • Status changed from new to closed

In 28821:

Multiple strings in .editorconfig section names should be wrapped in curly brackets.

props treyhunner, netweb.
fixes #28494.

Note: See TracTickets for help on using tickets.