Problem

  • Getting an editor's/IDE's configuration right is cumbersome and the first barrier for contributing to Drupal.

Goal

  • Automatically configure editors through .editorconfig.

Details

CommentFileSizeAuthor
#25 drupal-7.x-editorconfig-1713662-25.patch469 bytesjweowu
#13 core-editorconfig-1713662-13.patch577 bytesnod_
#6 editorconfig.6.patch626 bytesEric_A
#2 editorconfig.1.patch626 bytestreyhunner
editorconfig.0.patch775 bytessun
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

wjaspers’s picture

wjaspers CreditAttribution: wjaspers commented

I don't know where you find these amazing gems, but thank you!
Very handy!

treyhunner’s picture

treyhunner CreditAttribution: treyhunner commented
FileSize
editorconfig.1.patch626 bytes

I made a modified patch that removes some redundant/unecessary EditorConfig properties.

I removed the `byte_order_mark` property because it has been combine with the `charset` property (UTF-8 will always be assumed to have no byte order mark).

I removed the `indent_blank_lines` property because this property will probably not be implemented and its value is redundant (setting `trim_trailing_whitespace = true` will never indent blank lines).

I would also recommend removing the `curly_bracket_next_line`, `spaces_around_operators`, and `spaces_around_brackets` properties because they may never be implemented by any EditorConfig tools and their names may change if they are. I have removed these properties from the modified patch.

Status: Needs review » Needs work
Issue tags: -Contributor Experience (CX)

The last submitted patch, editorconfig.1.patch, failed testing.

treyhunner’s picture

treyhunner CreditAttribution: treyhunner commented
Status: Needs work » Needs review

#2: editorconfig.1.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Contributor Experience (CX)

The last submitted patch, editorconfig.1.patch, failed testing.

Eric_A’s picture

Eric_A CreditAttribution: Eric_A commented
Status: Needs work » Needs review
FileSize
editorconfig.6.patch626 bytes

This is #2 "uncorrupted".

xuhdev’s picture

xuhdev CreditAttribution: xuhdev commented
Issue tags: -Contributor Experience (CX)

#6: editorconfig.6.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Contributor Experience (CX)

The last submitted patch, editorconfig.6.patch, failed testing.

Eric_A’s picture

Eric_A CreditAttribution: Eric_A commented
Status: Needs work » Needs review

#1779638: Unexplained test failure in LocaleFileImportStatus->testBulkImportUpdateExisting()

Eric_A’s picture

Eric_A CreditAttribution: Eric_A commented

#6: editorconfig.6.patch queued for re-testing.

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ commented

Marked #1807722: Add EditorConfig configuration for text editors and #1873314: Add a .editorconfig file to ease working on PHP projects with different conventions than Drupal as dup of this one.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented

Would be nice to move forward here.

As of now, actual support for .editorconfig in editors is rather sparse, but that should not hold us up from committing this.

In fact, committing this is rather a "statement" and support for the generic idea of the entire developer/programmer community to introduce a standard mechanism. The more projects will do it, the better are the chances that something sophisticated will emerge.

I'm fine with the changes in #2. Perhaps we just want to remove the @todo from the patch in #6 and call this done?

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ commented
FileSize
core-editorconfig-1713662-13.patch577 bytes

Agreed.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran commented

+1 for RTBC

Crell’s picture

Crell CreditAttribution: Crell commented
Status: Needs review » Reviewed & tested by the community

Amen and pass the sweet potatoes.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Category: feature » task
Status: Reviewed & tested by the community » Fixed

Since this accelerates contribution to Drupal, re-classifying this as a task rather than a feature. Nice initiative; I hadn't heard of this before!

Committed and pushed to 8.x. Thanks!

quicksketch’s picture

quicksketch
he/him
CreditAttribution: quicksketch commented

Hm, just found this file after a git pull. I honestly thought someone had accidentally committed an IDE or text editor config file (since that is what this is, after all). Isn't it a bit early to commit something that no text editors or IDEs support natively (as in without a separately downloaded plugin)? Do we have any idea if this will ever actually do something? It seems like a good idea but there's little evidence this will become common. I suppose this is a chicken-egg problem, so lets be open to removing this in the future if broad support remains absent.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Issue tags: +revisit before beta

Sure, that sounds reasonable. Tagging "revisit before release" which is the standard "we might live to regret this" tag. :D

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ commented

Since I know at least chx and me uses PHPStorm: http://youtrack.jetbrains.com/issue/IDEA-87499

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

JohnAlbin’s picture

JohnAlbin
he/they/他
Primary language English
Location Taipei, Taiwan
CreditAttribution: JohnAlbin commented

Added Drupal to the list of projects supporting .editorconfig. https://github.com/editorconfig/editorconfig/wiki/Projects-Using-EditorC...

fuzzy76’s picture

fuzzy76 CreditAttribution: fuzzy76 commented

What about a D7 backport?

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Issue summary: View changes
Issue tags: -revisit before beta +revisit before release candidate
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Issue tags: -revisit before release candidate

This has been in for three years and a lot more people are using phpstorm than then (although not me!), so untagging.

jweowu’s picture

jweowu CreditAttribution: jweowu commented
Version: 8.0.x-dev » 7.x-dev
Status: Closed (fixed) » Needs review
Issue tags: +Needs backport to D7
FileSize
drupal-7.x-editorconfig-1713662-25.patch469 bytes

Seconding fuzzy76's comment #22: "What about a D7 backport?"

I see nothing in the D8 .editorconfig file that isn't also applicable to D7 coding standards (it covers only a very few basics, after all), so I see no reason why this couldn't be committed to D7 as-is, for the same benefits?

Status: Needs review » Needs work

The last submitted patch, 25: drupal-7.x-editorconfig-1713662-25.patch, failed testing.

jweowu’s picture

jweowu CreditAttribution: jweowu commented
Status: Needs work » Needs review

The testbot is obviously having a bad day.

jweowu’s picture

jweowu CreditAttribution: jweowu commented

I triggered the tests again for good measure, just so it shows a pass (which it now does).

I feel like this should be a very easy RTBC by virtually anyone involved with this issue? It's the exact same file being added for the exact same reasons.

basvredeling’s picture

basvredeling
Primary language Dutch
Location Amsterdam
CreditAttribution: basvredeling at Code Culture commented
Status: Needs review » Reviewed & tested by the community

Tested in PHPstorm, works fine.

Fabianx’s picture

Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commented
Issue tags: +Pending Drupal 7 commit

I am perfectly fine with that going into Drupal 7 as well.

Marked for commit.

stefan.r’s picture

stefan.r CreditAttribution: stefan.r commented
Issue tags: +7.50 release notes

  • Fabianx committed d516757 on 7.x 
    Issue #1713662 by nod_, Eric_A, treyhunner, sun, basvredeling: Introduce...
Fabianx’s picture

Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commented
Status: Reviewed & tested by the community » Fixed
Issue tags: -Pending Drupal 7 commit

Committed and pushed to 7.x! Thanks!

David_Rothstein’s picture

David_Rothstein CreditAttribution: David_Rothstein as a volunteer commented

Adding an issue credit for @jweowu on the Drupal 7 backport. (Possibly it went missing due to a drupal.org bug, not sure.)

Fabianx’s picture

Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commented

#34: Thanks, David!

FrittenKeeZ’s picture

FrittenKeeZ CreditAttribution: FrittenKeeZ commented
Status: Fixed » Needs review

Honestly, I wonder why you haven't included settings for markdown files, since they are used basically by everyone...
Removing trailing whitespace in markdown files will ruin line breaks.

[*.md]
trim_trailing_whitespace = false
Fabianx’s picture

Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commented
Status: Needs review » Fixed

#36: Thank you for the feedback, but please open a new issue against 8.1.x to fix it there. (and link it here)

For functionality present in both versions, we only can and do backport what has been committed to Drupal 8 per the backports policy.

Thank you!

FrittenKeeZ’s picture

FrittenKeeZ CreditAttribution: FrittenKeeZ commented

#37: Roger that!

I have created a new issue here: #2764829: EditorConfig file lack settings for markdown files

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.