12
202
Fork
You've already forked mitra
26

Address EditorConfig violations #242

Open
BaumiCoder wants to merge 7 commits from BaumiCoder/mitra:fix-editorconfig-violations into main
pull from: BaumiCoder/mitra:fix-editorconfig-violations
merge into: silverpill:main
silverpill:main
silverpill:minimitra-compat
silverpill:docker-image
silverpill:search-create-index
silverpill:media-storage-s3
First-time contributor
Copy link

This Pull Request is part of an academic research project. More preciously it is for my master's thesis, more background information here.

Definition: EditorConfig violations

The goal of this Pull Request is to address all EditorConfig violations in the project. With EditorConfig you have set for example trim_trailing_whitespace = true for most files, but there were files with trailing spaces. I call this an EditorConfig violation as a file content violates the respective EditorConfig property. They can have different kind of impacts. For example trailing spaces, which get remove with the next save, may make the next diff / Pull Request to this file less readable. Some other, like a mixture of tabs and spaces for indentation, can make code much harder to read. Like in this code block of your documentation
here (fixed with this PR).

How to address them?

To check for such violations and fix them I developed the CLI tool ecformat. However, there are different ways to address the violations:

  1. Adjust EditorConfig: The EditorConfig properties for a file can be wrong. For example, you have to use tab characters for indentations
    in a Makefile for correct syntax. Therefore, replacing these tabs with spaces would break it. The correct way to address that is to set indent_style = tab for them, which ensure that that tabs are used for indentations.
  2. Fix file content: Another way is to fix the content of the file. Which means for example for trim_trailing_whitespace = true to remove trailing spaces. To do so ecformat fix can be helpful.
  3. Ignore files: For some files it is not meaningful to consider the EditorConfig violations, which ecformat check finds. I added a .ecformat_ignore file for this. It has the common ignore syntax such as a .gitignore. To use such ignore files, provide the name of them with an CLI switch, ecformat check --ignore-file .ecformat_ignore. (The .gitignore files are automatically considered inside a git repository.) There are different reasons to ignore files:
    1. As EditorConfig is only for text files, considering binary files does not make sense.
    2. EditorConfig supports to set the property on the file level. However, there can be files with intentional differences inside. Such files need to be ignored fully by ecformat as it currently (version 0.2.0) has no support to ignore only a single property for some files. (There were no such cases in this project.)

Changes in this Pull Request

For this Pull Request, I addressed the violations in the project in the way, which looks the most correct one to me. (I focused on the Properties of the current EditorConfig specification 0.17.2.) If I fixed file contents, I manually reviewed all of them, making adjustments in the ecformat fix changes were necessary.

The only remaining violation is in the file contrib/openrc/mitra. It uses tab characters for indentation and not spaces as set for all files with EditorConfig. Do not know anything about this special file format. Therefore, I am not sure if it is safe to replace the tabs with spaces or if that can break anything. Please let me know if I should fix the file content or adjust the EditorConfig for this file.

If I picked the wrong way to address the violations for some other files, please let me know with a respective Review comment. Then I will address it in the correct way. I add Review comments myself for special parts, which are especially worthy of discussion. To support the Review of this Pull Request, I tried to group the changes in meaningful commits with respective commit messages. Therefore, may take a look at the commit history.

Avoid EditorConfig violations in the future

Such EditorConfig violations can creep in again. Possible reasons can be that the editor of some contributors has no EditorConfig support, or it would be required to install a respective plugin for the support.

If you want to avoid them in the future, I would suggest integrating a respective utility. For example as part of the CI. One possibility is to use ecformat with ecformat check --ignore-file .ecformat_ignore. Another one is editorconfig-checker, which is a more sophisticated tool without the possibility to fix violations. It provides more configuration options, such as ignoring violations in single lines with marker comments.

Let me know if you are interested using one of them. I could add one in this Pull Request or a follow-up Pull Request. If you not consider to use ecformat in your project, the .ecformat_ignore file is maybe not worth to merge into the master branch. I can remove it if desired.

This Pull Request is part of an academic research project. More preciously it is for my master's thesis, more background information [here](https://codeberg.org/BaumiCoder/Masterthesis). ### Definition: EditorConfig violations The goal of this Pull Request is to address all [EditorConfig](https://editorconfig.org) violations in the project. With EditorConfig you have set for example `trim_trailing_whitespace = true` for most files, but there were files with trailing spaces. I call this an EditorConfig violation as a file content violates the respective EditorConfig property. They can have different kind of impacts. For example trailing spaces, which get remove with the next save, may make the next diff / Pull Request to this file less readable. Some other, like a mixture of tabs and spaces for indentation, can make code much harder to read. Like in this code block of your documentation [here](https://codeberg.org/silverpill/mitra/src/commit/72d5b0b047da87a21af390264778310f30fbaec6/docs/reverse_proxy.md#etc-caddy-caddyfile) (fixed with this PR). ### How to address them? To check for such violations and fix them I developed the CLI tool [ecformat](https://codeberg.org/BaumiCoder/ecformat). However, there are different ways to address the violations: 1. **Adjust EditorConfig:** The EditorConfig properties for a file can be wrong. For example, you have to use tab characters for indentations in a Makefile for correct syntax. Therefore, replacing these tabs with spaces would break it. The correct way to address that is to set `indent_style = tab` for them, which ensure that that tabs are used for indentations. 2. **Fix file content:** Another way is to fix the content of the file. Which means for example for `trim_trailing_whitespace = true` to remove trailing spaces. To do so `ecformat fix` can be helpful. 3. **Ignore files:** For some files it is not meaningful to consider the EditorConfig violations, which `ecformat check` finds. I added a `.ecformat_ignore` file for this. It has the common ignore syntax such as a `.gitignore`. To use such ignore files, provide the name of them with an CLI switch, `ecformat check --ignore-file .ecformat_ignore`. (The `.gitignore` files are automatically considered inside a git repository.) There are different reasons to ignore files: 1. As EditorConfig is only for text files, considering binary files does not make sense. 2. EditorConfig supports to set the property on the file level. However, there can be files with intentional differences inside. Such files need to be ignored fully by `ecformat` as it currently (version 0.2.0) has no support to ignore only a single property for some files. (There were no such cases in this project.) ### Changes in this Pull Request For this Pull Request, I addressed the violations in the project in the way, which looks the most correct one to me. (I focused on the Properties of the current [EditorConfig specification](https://spec.editorconfig.org/#supported-pairs) 0.17.2.) If I fixed file contents, I manually reviewed all of them, making adjustments in the `ecformat fix` changes were necessary. The only remaining violation is in the file [contrib/openrc/mitra](https://codeberg.org/silverpill/mitra/src/commit/72d5b0b047da87a21af390264778310f30fbaec6/contrib/openrc/mitra). It uses tab characters for indentation and not spaces as set for all files with EditorConfig. Do not know anything about this special file format. Therefore, I am not sure if it is safe to replace the tabs with spaces or if that can break anything. Please let me know if I should fix the file content or adjust the EditorConfig for this file. If I picked the wrong way to address the violations for some other files, please let me know with a respective Review comment. Then I will address it in the correct way. I add Review comments myself for special parts, which are especially worthy of discussion. To support the Review of this Pull Request, I tried to group the changes in meaningful commits with respective commit messages. Therefore, may take a look at the commit history. ### Avoid EditorConfig violations in the future Such EditorConfig violations can creep in again. Possible reasons can be that the editor of some contributors has no EditorConfig support, or it would be required to install a respective plugin for the support. If you want to avoid them in the future, I would suggest integrating a respective utility. For example as part of the CI. One possibility is to use `ecformat` with `ecformat check --ignore-file .ecformat_ignore`. Another one is [editorconfig-checker](https://editorconfig-checker.github.io), which is a more sophisticated tool without the possibility to fix violations. It provides more configuration options, such as ignoring violations in single lines with marker comments. Let me know if you are interested using one of them. I could add one in this Pull Request or a follow-up Pull Request. If you not consider to use `ecformat` in your project, the `.ecformat_ignore` file is maybe not worth to merge into the master branch. I can remove it if desired.
@ -6,6 +6,8 @@ trim_trailing_whitespace = true
[*.rs]
indent_size = 4
# Do not remove the two trailing spaces for a line break in doc comments.
Author
First-time contributor
Copy link

With EditorConfig it is not possible to only trim trailing whitespace outside of doc comments. If you want to avoid trailing whitespace there, I would see two possible ways:

  1. Use <br> for line breaks in doc comments. Then trim_trailing_whitespace = true could not break the formatting of the documentation anymore.
  2. Use rustfmt in the project. I would assume that it can handle this Rust specific need. However, applying it to all files, with cargo fmt, makes many small changes across the whole code base. Therefore, introducing it to the project should be handled with care as it has a high merge conflict potential, and you may want to configure it to your preferred (and already widely used) style for the project..
With EditorConfig it is not possible to only trim trailing whitespace outside of doc comments. If you want to avoid trailing whitespace there, I would see two possible ways: 1. Use `<br>` for line breaks in doc comments. Then `trim_trailing_whitespace = true` could not break the formatting of the documentation anymore. 2. Use rustfmt in the project. I would assume that it can handle this Rust specific need. However, applying it to all files, with `cargo fmt`, makes many small changes across the whole code base. Therefore, introducing it to the project should be handled with care as it has a high merge conflict potential, and you may want to configure it to your preferred (and already widely used) style for the project..
@ -0,0 +1,5 @@
# Markdown file with an Caddyfile in an code block.
[reverse_proxy.md]
# "caddy fmt" uses tabs for Caddyfile.
Author
First-time contributor
Copy link

I don't know if using 4 spaces would technically work when somebody copies this "template" for actual use. However, following caddy's default formatter seems reasonable to me.

I don't know if using 4 spaces would technically work when somebody copies this "template" for actual use. However, following caddy's default formatter seems reasonable to me.

Hello! Cool project - thanks for the pull request.

The only remaining violation is in the file contrib/openrc/mitra. It uses tab characters for indentation and not spaces as set for all files with EditorConfig. Do not know anything about this special file format. Therefore, I am not sure if it is safe to replace the tabs with spaces or if that can break anything. Please let me know if I should fix the file content or adjust the EditorConfig for this file.

Let's ask @caohuak, who added this file.

Hello! Cool project - thanks for the pull request. > The only remaining violation is in the file contrib/openrc/mitra. It uses tab characters for indentation and not spaces as set for all files with EditorConfig. Do not know anything about this special file format. Therefore, I am not sure if it is safe to replace the tabs with spaces or if that can break anything. Please let me know if I should fix the file content or adjust the EditorConfig for this file. Let's ask @caohuak, who added this file.
.ecformat_ignore Outdated
@ -0,0 +1,2 @@
# Binary files
*.png

A separate configuration file is not ideal, it makes the file list longer. Have you considered putting the ignorelist to the root .editorconfig? It looks like extensions are allowed: https://github.com/editorconfig/editorconfig/issues/463

A separate configuration file is not ideal, it makes the file list longer. Have you considered putting the ignorelist to the root `.editorconfig`? It looks like extensions are allowed: https://github.com/editorconfig/editorconfig/issues/463
Author
First-time contributor
Copy link

Thanks for that suggest. I created an issue for it in my project: BaumiCoder/ecformat#58

However, there is already some (hacky) way to "ignore" it with the default EditorConfig possibilities. Currently, you set your default settings for all files with the [*] section, which is a common approach. By definition this also applies to *.png files, although nobody opens an PNG file with an editor and therefore this does not really matter in most cases. As it matters for the special case of ecformat, we would simply not set any EditorConfig properties for these files my changing it to a [!*.png] section. This would set properties for all files, except for *.png files. An explicit property to ignore it for ecformat will be of course a nicer solution and will allow setting properties for some files but still let ecformat ignore them.

Thanks for that suggest. I created an issue for it in my project: https://codeberg.org/BaumiCoder/ecformat/issues/58 However, there is already some (hacky) way to "ignore" it with the default EditorConfig possibilities. Currently, you set your default settings for all files with the `[*]` section, which is a common approach. By definition this also applies to `*.png` files, although nobody opens an PNG file with an editor and therefore this does not really matter in most cases. As it matters for the special case of ecformat, we would simply not set any EditorConfig properties for these files my changing it to a `[!*.png]` section. This would set properties for all files, except for `*.png` files. An explicit property to ignore it for ecformat will be of course a nicer solution and will allow setting properties for some files but still let ecformat ignore them.

I think [!*.png] is a good solution.

I think `[!*.png]` is a good solution.
Author
First-time contributor
Copy link

Done with 6501381ae8

While testing it, I found that the syntax is even a bit different. It works correct with [*[!.png]].

Done with 6501381ae8636326e971d9e69836eddfd6d0fab5 While testing it, I found that the syntax is even a bit different. It works correct with `[*[!.png]]`.
BaumiCoder marked this conversation as resolved
Contributor
Copy link

@silverpill wrote in #242 (comment):

Hello! Cool project - thanks for the pull request.

The only remaining violation is in the file contrib/openrc/mitra. It uses tab characters for indentation and not spaces as set for all files with EditorConfig. Do not know anything about this special file format. Therefore, I am not sure if it is safe to replace the tabs with spaces or if that can break anything. Please let me know if I should fix the file content or adjust the EditorConfig for this file.

Let's ask @caohuak, who added this file.

This is an openrc script, which is a specific type of shell script, so you can safely replace all tabs with spaces.

@silverpill wrote in https://codeberg.org/silverpill/mitra/pulls/242#issuecomment-18454019: > Hello! Cool project - thanks for the pull request. > > > The only remaining violation is in the file contrib/openrc/mitra. It uses tab characters for indentation and not spaces as set for all files with EditorConfig. Do not know anything about this special file format. Therefore, I am not sure if it is safe to replace the tabs with spaces or if that can break anything. Please let me know if I should fix the file content or adjust the EditorConfig for this file. > > Let's ask @caohuak, who added this file. This is an openrc script, which is a specific type of shell script, so you can safely replace all tabs with spaces.
Do not set properties for binary files. This avoids the need to ignore
them for ecformat explicitly.
Fix indentation style in mitra file
Some checks are pending
ci/woodpecker/pr/test Pipeline is pending approval
7c53a7b9af
Author
First-time contributor
Copy link

This is an openrc script, which is a specific type of shell script, so you can safely replace all tabs with spaces.

Okay, I replaced the tabs with commit 7c53a7b9af

> This is an openrc script, which is a specific type of shell script, so you can safely replace all tabs with spaces. Okay, I replaced the tabs with commit 7c53a7b9af582897c5a26750bd2d3214c2ffeec6
Some checks are pending
ci/woodpecker/pr/test Pipeline is pending approval
This pull request can be merged automatically.
This branch is out-of-date with the base branch
You are not authorized to merge this pull request.
View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u fix-editorconfig-violations:BaumiCoder-fix-editorconfig-violations
git switch BaumiCoder-fix-editorconfig-violations

Merge

Merge the changes and update on Forgejo.

Warning: The "Autodetect manual merge" setting is not enabled for this repository, you will have to mark this pull request as manually merged afterwards.

git switch main
git merge --no-ff BaumiCoder-fix-editorconfig-violations
git switch BaumiCoder-fix-editorconfig-violations
git rebase main
git switch main
git merge --ff-only BaumiCoder-fix-editorconfig-violations
git switch BaumiCoder-fix-editorconfig-violations
git rebase main
git switch main
git merge --no-ff BaumiCoder-fix-editorconfig-violations
git switch main
git merge --squash BaumiCoder-fix-editorconfig-violations
git switch main
git merge --ff-only BaumiCoder-fix-editorconfig-violations
git switch main
git merge BaumiCoder-fix-editorconfig-violations
git push origin main
Sign in to join this conversation.
No reviewers
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
3 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
silverpill/mitra!242
Reference in a new issue
silverpill/mitra
No description provided.
Delete branch "BaumiCoder/mitra:fix-editorconfig-violations"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?