Skip to content
This repository has been archived by the owner on Mar 29, 2022. It is now read-only.

Introduce editorconfig #694

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

the-spectator
Copy link
Contributor

@the-spectator the-spectator commented Oct 17, 2020

Description

This PR tries to partially address #623 by adding .editorconfig. Editorconfig helps to maintain consistent file formatting across various editors and IDEs.

Resource

Test process

  • Trims trailing spaces
  • Follows tabindent=2
  • Uses spaces instead of tabs. (Can be configured 😅 )

Requirements to merge

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code in hard-to-understand areas
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

Copy link
Member

@MattIPv4 MattIPv4 left a comment

Choose a reason for hiding this comment

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

Would you be interested in adding something to the CI to ensure the editorconfig is followed across all files?

.editorconfig Outdated
Comment on lines 13 to 12
[{*.{rb,erb,js,coffee,json,yml,css,scss,sh,markdown,md,html}]
indent_size = 2
Copy link
Member

Choose a reason for hiding this comment

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

This seems like an odd way to set the indent. Why not set indent_size = 2 for everything, and then set a different indent size for anything that doesn't fit that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense. I will update accordingly.

@the-spectator the-spectator force-pushed the introduce_editor_config branch 3 times, most recently from 91ae00d to 5167e3c Compare October 19, 2020 17:11
@the-spectator the-spectator force-pushed the introduce_editor_config branch from 5167e3c to ce63a4d Compare October 19, 2020 17:21
@the-spectator
Copy link
Contributor Author

@MattIPv4 I have updated according to your review and added editorconfig lint GitHub action.

@@ -19,3 +19,5 @@ jobs:
gem install bundler dotenv
bundle install --jobs 4 --retry 3
bundle exec rubocop
- name: Enforce EditorConfig
uses: zbeekman/EditorConfig-Action@28b76e718c351dd35b22c8f7216192bb9de5c21b
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Had to use SHA because of npm audit issue in action zbeekman/EditorConfig-Action#35

Copy link
Member

@MattIPv4 MattIPv4 left a comment

Choose a reason for hiding this comment

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

It'd be nice if this could be added to script/test also, so it might be best to roll our own solution instead of relying on a third-party action?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants