Better EditorConfig Settings & Code Styles Validation via Travis CI

[tajmone]

I've managed to revise the EditorConfig and gitattribute setting, as discussed in #6520.

So far, I've covered all the main file types of the repository, leaving out some extensions (HTML, XML, and autogenerated files) which either might not be so important to cover, or might display different code styles depending on context (e.g. XML files, which are present with multiple file extensions).

The list of covered file types:

• Repository configurations (Git settings, EditorConfig, and CI services)
• Shell scripts
• Make files
• Haskell sources
• Markdown docs (as discussed in Improve EditorConfig Settings #6520, and excluding test/ folder)
• Lua and Perl scripts

I've unset validation rules for any files inside the test/ folder, to allow testing edge cases and malformed sources.

A few cosmetic tweaks to some sources were required (indentations, etc.) to pass Travis CI validation via EClint, and now all tests pass.

I'd like the PR to be reviewed, to ensure that no important extensions are being left out from the settings, and that the current configuration is fine.

Also, we might need to decide if we should squash this PR into a single commit or keep each commit separate (I opt for the latter, for it provide a more readable history regarding the source tweaks that enforce code styles, on a per-type basis).

[tarleb]

Thank you for this! I don't know enough about gitattributes, but the editorconfig changes and validation look really good to me.
@jgm has to make the final decision.

[tarleb]

I think we're not using trailing spaces anywhere, might be ok to set this to true.

[tajmone]

I personally never use trailing spaces for hard line-breaks, but we need to check with @jgm for this setting, after all GFM does support that feature so potentially it could be used in the GFM documentation or templates, and if I remember correctly pandoc also supports the feature via extensions.

I always set trim_trailing_whitespace to true in my projects, because trailing space are the main cause of commits "noise", leading to hard-to-read diffs and (often) spurious contents changes.

[jgm]

I'm sure there are tests that include trailing whitespace, even if we don't use it in documentation (and for all I recall, we might).

[tajmone]

I've revised the rules for test/ folder in commit 6080aeb, so they apply to markdown files only — basically the only checks now are for UTF-8 encoding and EOL consistency, all other rules are dropped to allow testing edge-cases (including trailing spaces).

even if we don't use it in documentation (and for all I recall, we might).

I've checked. the only markdown file with trailing spaces is COPYING.md, which contains some trailing spaces that are just spurious and could actually be removed.

So, @jgm, should we set the rules for markdown files outside the test/ folder to forbid trailing WS? (and cleanup the COPYING.md doc)

Unless we need to use them in the documentation, it would prevent unclean commits.

[tarleb]

The general coding style uses two spaces, but Haskell indentation is weird sometimes. Does verification fail if we set this?

[tajmone]

Yes it would fail because the validator tool expects all indentation to be a multiple of the base value. Unfortunately the EditorConfig project doesn't offer separate settings for editor/IDEs configurations and validators right now.

Since most coders add extra spaces to obtain elegant alignments on code that spans multiple lines, it often becomes necessary to either unset indent_size or set it to 1 — the former is better because it doesn't override the editor's defaults, whereas the latter would actually enforce 1-space tabs.

As for markdown (and other lightweight markup syntaxes) the problem affect code blocks, which could be using even or odd indentation, depending on the language snippet being included.

Hopefully in future editions of EditorConfig we'll see new settings to address validation aspects only. Right now, one is forced to compromise on some features to obtain both editor settings and CI validation — e.g. EditorConfig doesn't (and will never) support end_of_line = native, so to allow cross-platform EOL normalization end_of_line has to be unset and the project has to rely on .gitattributes instead.

I'll take a further look at EClint's documentation, there are some ways to customize validation settings and overriding the .editorconfig values via a separate config file for EClint; if this is the case, we could enforce 2-spaces on the editor and override it on EClint by unsetting indent_size for some file types. It might require some time for me to dig into it and test it, but in the meantime we could use these settings for they are fairly good in practical use, from my own experience (you get a sound validation and don't interfere with the editor/IDE).

[tajmone]

I've checked EClint documentation, and there's no easy way to override .editorconfig settings on a per-file-type manner (you can override options globally for each run). The only alternative would be to run multiple EClint checks, each targetting a specific file extension, but it seems an overkill solution.

[tarleb]

I just realized that we still want the general rules to apply for Haskell and Lua files in test. Should we move those rules further down, or is there a better way?

[tajmone]

Maybe we should apply the above pattern only to markdown files?

## Test Files
#############
[test/**.{md,markdown}]

After all, I believe that is only the markdown files that need relaxed rules to allow testing edge-cases and malformed documents.

[tajmone]

Fixed in 6080aeb!

[tarleb]

If I understand correctly, then this is a temporary solution and would be converted into a GitHub Action workflow after merging?

[tajmone]

I leave that choice to you, as you're surely more qualified to decide on this.

Personally, I always use Travis for code validation, to avoid bumping into the free GH Actions limits (since validation is done for every commit on every branch, having many repositories could quickly reach the limit).

Another reason to keep it on Travis (instead of Circle or GH Actions) might be performance, since it would run in a parallel job.

[jgm]

I'd like to avoid using multiple CI providers; if we can do it all in GH actions that seems better to me.

[tajmone]

There's a ready availabe EClint action:

https://github.com/marketplace/actions/eclint

I haven't use GH Actions before, some I'm not sure how to set it up. Can someone else add the action?

[tarleb]

I don't understand most of the changes in this file. Does validation fail without them?

[tajmone]

Yes it fails because in this case the setting is 2 spaces, so all indentation needs to be a multiple of 2 (i.e. odd spacing).

All the (cosmetic) changes I've done to the sources were due to validations errors.

[tarleb]

This looks wrong.

[tajmone]

Fixed!

[tarleb]

These should be indented by two additional spaces.

[tajmone]

Commit 5bdf167 should fix this (hopefully).

[tarleb]

I think we are using 4 spaces, at least that's what I've been using. But I might be wrong and I am ok with either.

[tajmone]

To the best of my knowledge, shell scripts should use tabs only for indentation, because spaces indents could lead to problems in some contexts — the matter is subject of dispute (as everything relating to the tabs-vs-space flamewars) but this seems to be the prevalent opinion, at least on long StackOverflow discussions.

But I'll set it to any value you suggest, the most important thing is to keep the code consistent.

[tajmone]

The reason why tabs are preferable is in case shell scripts are used to dynamically generate other scripts via pipes and redirections, where tabs are a safer option because common used tools will auto-strip leading tabs (but not spaces).

An example:

https://www.howtobuildsoftware.com/index.php/how-do/ctWc/shell-sh-perforce-error-in-creating-p4-label-non-interactively-as-part-of-shell-script

Beside that, it boils down to a matter of personal choice (and the many flamewars on the subject didn't really bring any solution to the issue). I personally prefer using spaces in version controlled projects, for they create more readable diffs. But since in Make files tabs are mandatory, and shell scripts are sometimes used to manipulate Make files, I prefer to adopt tabs.

[tajmone]

Some interesting trivia on the subject:

• Programmers who use spaces instead of tabs make more money

So, yes, I'm for spaces-indentation, but make an exception for shell scripts.

[tarleb]

Interesting, I didn't know. Thanks!

[jgm]

I'm not sure about the impact of all these changes. What if someone edits a template and this adds CRLF line endings, and then runs tests and the tests fail to pass because the golden tests have LF line endings? We don't want that.

[tajmone]

What if someone edits a template and this adds CRLF line endings, and then runs tests and the tests fail to pass because the golden tests have LF line endings? We don't want that.

Currently the EditorConfig settings are enforcing only LF EOL for repository configuration files, Make files and shell scripts; all other text files EOLs are set to native via Git attributes and are being ignored by the EditorConfig validator.

Of course, Git will prevent commits in case of inconsistent EOLs.

[tarleb]

I honestly don't understand why, but the tests still pass even after running unix2dos data/templates/*.

[tajmone]

I honestly don't understand why, but the tests still pass even after running unix2dos data/templates/*.

Because EditorConfig doesn't support the native value for line-ending settings:

• AutoCrlf support (end_of_line = native) editorconfig/editorconfig#226
• Enforce EOL-Agnostic Consistency Checks editorconfig/editorconfig#431

There's been a long debate on the lack of this feature, but (to make it short) the official response is that "native" is not acceptable as an EditorConfig definition, so it won't be implemented.

So when working with Git repos that allow native EOL normalization (which should be the default) you simply have to disable EOL consistency in EditorConfig: end_of_line = unset.

Beside those file-types which need to enforce a specific EOL type, we're relying on .gitattributes for checking native EOL consistency at commit time.
Which is why running unix2dos/dos2unix locally won't fail the validation test — but Git should refuse a commit which would introduce wrong or inconsistent EOLs. So it's safe to rely on.