Skip to content

Conversation

@hyperupcall
Copy link
Collaborator

@hyperupcall hyperupcall commented Jan 14, 2023

While working on #1021, I noticed that some files like git-delta don't end in a newline. As a result, that last "line" (git diff --name-only --diff-filter=$filter $branch) is not considered part of the text file, according to POSIX.

To fix this, I propose adding an EditorConfig file. When editors read this (some require a plugin like VSCode) file, it adjusts the formatting based on the rules. One thing to note is that rules that specify whitespace are only take affect to each inserted text. For example, with indent_style = space, when editing a file that predominately uses tabs, only the spaces will be inserted on each <tab> - other characters are not modified.

It seems both 2 and 4-space shell scripts are used in this repository, so I decided to omit that constraint from the .editorconfig file.

@hyperupcall hyperupcall changed the title Add EditorConfig Add EditorConfig file Jan 14, 2023
@hyperupcall hyperupcall force-pushed the add-editorconfig branch 2 times, most recently from 3e9789d to b9d0072 Compare January 14, 2023 10:58
Copy link
Collaborator

@spacewander spacewander left a comment

Choose a reason for hiding this comment

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

Can we add a job to check it in the CI?

@hyperupcall
Copy link
Collaborator Author

Is there anything in particular you wish to check? For consistent formatting for all of the files then maybe shfmt can be added in a different PR with CI. But EditorConfig isn't really the tool for that and there doesn't exist any first-party tools for checking EditorConfig rules.

@spacewander
Copy link
Collaborator

@hyperupcall
Copy link
Collaborator Author

hyperupcall commented Jan 17, 2023

editorconfig-checker is a third-party tool so I was hesitant to add it. Since there isn't an autofix mode, there is no automatic way of it conforming to the .editorconfig without something like shfmt to do the heavy lifting for us.

@hyperupcall
Copy link
Collaborator Author

What are your thoughts on shfmt?

@spacewander
Copy link
Collaborator

I don't like a formatter, which forces other people to use a style that is not used in their daily life.

@hyperupcall
Copy link
Collaborator Author

Fair enough - I don't use shfmt either.

How about adding editorconfig-checker to CI to only run on changes files?

@spacewander
Copy link
Collaborator

Fair enough - I don't use shfmt either.

How about adding editorconfig-checker to CI to only run on changes files?

LGTM

@hyperupcall
Copy link
Collaborator Author

Epic 👍

I also changed Ubuntu version since 18.04 is being currently phased out with brownouts

steps:
- name: 'Get Changed Files'
id: 'files'
uses: 'jitterbit/get-changed-files@v1'
Copy link
Collaborator

Choose a reason for hiding this comment

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

I see some warnings in https://github.com/tj/git-extras/actions/runs/3979079395/jobs/6821262963

Warning: The `set-output` command is deprecated and will be disabled soon. Please upgrade to using Environment Files. For more information see: https://github.blog/changelog/2022-10-11-github-actions-deprecating-save-state-and-set-output-commands/

Can it be avoided?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I switched to masesgroup/retrieve-changed-files@v2, which fixed this issue

branches: [master]
pull_request:
branches: [ master ]
branches: ['*']
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's change it back once this PR is approved.

@hyperupcall hyperupcall force-pushed the add-editorconfig branch 2 times, most recently from 21588bb to ca1b05e Compare January 26, 2023 00:11
@hyperupcall
Copy link
Collaborator Author

The lint checks work with the new changes.

I also changed the pull_request.branches back, but I'm not sure how contributors are supposed to know if their files are formatted correctly

@spacewander
Copy link
Collaborator

The lint checks work with the new changes.

I also changed the pull_request.branches back, but I'm not sure how contributors are supposed to know if their files are formatted correctly

They can check this locally? It would be good to add an editorconfig plugin in the editor. Anyway, they can also figure it out by reading the output of the CI.

@hyperupcall
Copy link
Collaborator Author

hyperupcall commented Jan 26, 2023

Oh oops 🤦 - for some reason I thought that CI wouldn't run if pull_request.branches wasn't *. But I now see that the other PRs show CI.

Is this ready to go?

@spacewander
Copy link
Collaborator

Oh oops 🤦 - for some reason I thought that CI wouldn't run if pull_request.branches wasn't *. But I now see that the other PRs show CI.

Is this ready to go?

Merged. Thanks!

@spacewander spacewander merged commit 589ef33 into tj:master Jan 27, 2023
@hyperupcall hyperupcall deleted the add-editorconfig branch January 27, 2023 12:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants