Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update: [Dev] Improve .editorconfig with C++ formatting rules based on OpenTTD code style #10104

Merged
merged 2 commits into from Apr 9, 2023
Merged

Update: [Dev] Improve .editorconfig with C++ formatting rules based on OpenTTD code style #10104

merged 2 commits into from Apr 9, 2023

Conversation

Arastais
Copy link
Contributor

@Arastais Arastais commented Oct 24, 2022

Motivation / Problem

The current .editorconfig file is very limited and doesn't provide any formatting rules for C++ files. This can cause IDEs to auto-format based on a developer's setting, causing an extra hassle for either the developer and even maintainers if it goes unnoticed.

Description

This PR only adds almost all possible C++ specific formatting rules to the repo's .editorconfig file and sets their values to best match OpenTTD. It only touches the .editorconfig file and is simply a quality-of-life addition for developers/maintainers - it will not be seen by players at all.

Each rule has an end-of-line comment describing what the key-value combination does. The file is sectioned and ordered by the same ordering on the respective listed web pages for the rules/keys.

Most IDEs should support these formatting rules, and the mainstream ones (VS, VS Code, IntelliJ/ReSharper, etc.) for sure do. Worst case (if a developer's IDE doesn't support these rules), then they're the same as before.

This will prevent developers from having to manually fix incorrect formatting (including wrong auto-formatting done by the IDE) or forget to write in the OpenTTD code style. This will also mean that maintainers will have to review/suggest code style changes or mistakes as often. Overall, I think this an important quality-of-life improvement that will save both developers and maintainers a lot of time and hassle.

Limitations

Obviously, not all rules in OpenTTD's coding style can be automatically followed by the IDE with just the .editorconfig, but this will prevent any wrong auto-formatting for sure.

Several rules were "skipped" (i.e. not given an value and thus will not override developers' settings for those rules) for a given reason. These reasons are listed in the comments of the rule.

indent_size, tab_width, and end_of_line were not given rules, as they are global and not clearly specified in OpenTTD's code style

A good way to review this is to make a test C++ source file and make code style mistakes on purpose, especially common ones, and see if the auto-formatter will correct them as expected.

Checklist for review

Some things are not automated, and forgotten often. This list is a reminder for the reviewers.

  • The bug fix is important enough to be backported? (label: 'backport requested')
  • This PR touches english.txt or translations? Check the guidelines
  • This PR affects the save game format? (label 'savegame upgrade')
  • This PR affects the GS/AI API? (label 'needs review: Script API')
    • ai_changelog.hpp, gs_changelog.hpp need updating.
    • The compatibility wrappers (compat_*.nut) need updating.
  • This PR affects the NewGRF API? (label 'needs review: NewGRF')

@PeterN
Copy link
Member

PeterN commented Oct 24, 2022

The EditorConfig specification says comments should go on individual lines, can you please move them up.

@LordAro
Copy link
Member

LordAro commented Oct 24, 2022

Also with an additional space after #

@PeterN
Copy link
Member

PeterN commented Oct 24, 2022

I tested auto formatting one source file and the only bits it got wrong are where we randomly decide to indent the right hand side of assignments, which I wouldn't expect any editor to magically guess what we want. Nice.

.editorconfig Outdated Show resolved Hide resolved
@Arastais
Copy link
Contributor Author

Arastais commented Oct 24, 2022

The EditorConfig specification says comments should go on individual lines, can you please move them up.

While the specification does say this, I didn't do this on purpose because it makes the file much harder to read (unless maybe you add spaces between each line, but it's not as good as having end-of-line comments). Likewise, the specification is based on the .ini format which uses semicolon as in-line comments, and thus the EditorConfig specification also states "A semicolon character (;) starts a line comment that terminates at the end of the line". So, I've went ahead and changed the # symbols to ;, which I feel is a good compromise between adhering to the specification and making the file readable.

I've also gone ahead and added a space before each comment symbol like @LordAro said.

@Arastais
Copy link
Contributor Author

Turns out the JetBrains IDEs (namely IntelliJ IDEA, ReSharper, and Rider) have a lot more C++ specific rules than visual studio, so I've added all of them.

Feel free to test it again (even in visual studio to check for unexpected behavior), but it should be good to go.

Copy link
Contributor

@rubidium42 rubidium42 left a comment

Choose a reason for hiding this comment

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

I haven't tested it, so there might be more cases where things are wrong. Though I like the idea of codifying the coding style in this way, as long as the tooling helps.

.editorconfig Outdated Show resolved Hide resolved
.editorconfig Outdated Show resolved Hide resolved
.editorconfig Outdated Show resolved Hide resolved
.editorconfig Outdated Show resolved Hide resolved
.editorconfig Outdated Show resolved Hide resolved
.editorconfig Outdated Show resolved Hide resolved
.editorconfig Outdated Show resolved Hide resolved
.editorconfig Outdated Show resolved Hide resolved
.editorconfig Outdated Show resolved Hide resolved
@2TallTyler 2TallTyler added the work: minor details This Pull Request has some minor details left to do label Feb 26, 2023
@Arastais
Copy link
Contributor Author

@rubidium42 I fixed the incorrect rules you pointed out; Thanks for checking those. I don't use resharper so didn't have an incentive to check and also didn't double-check those because I was essentially copying the rules from the visual studio section.

Too bad I didn't spell check 😵

@rubidium42
Copy link
Contributor

I just pushed a simple change to move the inline comments, which are forbidden by the specifications as mentioned before by @PeterN, to the line before it.
I'm not sure what @LordAro meant exactly with the comment about the space after the #. For now I made proper comments have the space after the # and cases where the configuration is commented out without a space after the #.

@2TallTyler 2TallTyler removed the work: minor details This Pull Request has some minor details left to do label Mar 3, 2023
Copy link
Member

@2TallTyler 2TallTyler left a comment

Choose a reason for hiding this comment

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

I don't have an encyclopedic knowledge of our coding style but I trust that @rubidium42 does. 😛 Comments look fine to me, I can read it just fine.

@rubidium42 rubidium42 merged commit 770df65 into OpenTTD:master Apr 9, 2023
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.

None yet

6 participants