Skip to content

Add .editorconfig#724

Merged
mblaney merged 5 commits intosimplepie:masterfrom
Alkarex:EditorConfig
Mar 31, 2022
Merged

Add .editorconfig#724
mblaney merged 5 commits intosimplepie:masterfrom
Alkarex:EditorConfig

Conversation

@Alkarex
Copy link
Contributor

@Alkarex Alkarex commented Feb 27, 2022

See https://editorconfig.org/

Helps text editors automatically adopt the conventions (e.g. for white-space) used in this specific project.

And .gitattributes for consistency: ensures line endings are not transformed when working on the same folder from different systems, e.g. WSL / Windows

See https://editorconfig.org/
And .gitattributes for consistency (ensures line endings are not transformed when working on the same folder from different systems, e.g. WSL / Windows)
.editorconfig Outdated
Comment on lines +1 to +27
# https://editorconfig.org

[*]
charset = utf-8
end_of_line = lf
insert_final_newline = true
trim_trailing_whitespace = true

[*.css]
indent_style = tab

[*.js]
indent_style = tab

[*.json]
indent_style = tab

[*.{markdown,md}]
indent_size = 4
indent_style = space

[*.sql]
indent_style = tab

[*.yml]
indent_size = 2
indent_style = space
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would recommend a couple of improvements:

This is the top-level definition, so let's mark it as root.

root = true

You can combine these for efficiency:

[*.{css,html,js,json,sql}]
indent_style = tab

Use both standard file extensions:

[*.{yaml,yml}]
indent_size = 2
indent_style = space

Markdown has some additional rules that are important:

[*.{markdown,md}]
indent_size = 4
indent_style = space
max_line_length = 0
trim_trailing_whitespace = false

Merge these in, and I'm happy to approve.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will do as you ask, but here is nevertheless some rationale for the current choices:

  • root = true prevents users from choosing the display size of their tab indentations (the idea by not having this rule being that users can have an even higher .editorconfig with cosmetic preferences)
  • [*.{css,html,js,json,sql}] fine but makes it more difficult to have distinct rules per language
  • [*.{yaml,yml}] there is no *.yaml file in this project
  • max_line_length does not exist, does it?
  • trim_trailing_whitespace = false although possible to want trailing whitespace in Markdown, it is not recommended anymore as far as I know, and I do not think SimplePie makes conscious use of it. e.g. https://github.com/DavidAnson/markdownlint/blob/main/doc/Rules.md#md009

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@skyzyx Requested changes applied 878ce44 but my comments still stand
As well as for the non-official max_line_length (off, not 0) aa11074

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • root = true prevents users from choosing the display size of their tab indentations (the idea by not having this rule being that users can have an even higher .editorconfig with cosmetic preferences)

Ah. I thought we'd gotten past this by now. PSR-1 and PSR-2 have been out for about a decade, and I'm just now realizing that SimplePie never adopted them. OK. We can remove this.

  • [*.{css,html,js,json,sql}] fine but makes it more difficult to have distinct rules per language

You would only collapse them as it makes sense. You expand if it makes sense. In the case of these rules, this is the most collapsed form which makes sense.

  • [*.{yaml,yml}] there is no *.yaml file in this project

Sure. This isn't a big deal, it just covers both common file extensions.

  • max_line_length does not exist, does it?

It's supported on EditorConfig.org. And at one point it was part of the official SimplePie coding style guide that the line length was 120 characters. But I haven't been a code contributor on this project since 2010. So YMMV.

I would say it should definitely be enabled for everything except for Markdown.

As for Markdown specifically, you can mark a stanza linebreak with either two spaces ( ), or a space followed by a backslash ( \). This is a very specific usage that isn't super-common (i.e., stanzas). But we don't want to break it when it's necessary.

If we agree to use the latter format ( \) instead of the former format ( ), then we can enable this one as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have just removed the root constraint as agreed 8d6af85

Regarding whitespace for line breaks, a search shows that this is not something used in the current SimplePie Markdown files. There are five instances of single trailing spaces in https://github.com/simplepie/simplepie/blob/master/CHANGELOG.md but those are clearly mistakes. I would vote for the backslash syntax \ or explicit <br />.

@mblaney mblaney merged commit d0bf34a into simplepie:master Mar 31, 2022
@Alkarex Alkarex deleted the EditorConfig branch March 31, 2022 09:48
Alkarex added a commit to FreshRSS/simplepie that referenced this pull request Aug 30, 2022
Fix regression from simplepie#738
See simplepie#724

Sad to see all these wasted / inefficient / hard-coded spaces though... Tabs ❤️
mblaney pushed a commit that referenced this pull request Sep 22, 2022
Fix regression from #738
See #724

Sad to see all these wasted / inefficient / hard-coded spaces though... Tabs ❤️
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants