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

Auto-format code #291

Merged
merged 8 commits into from
Dec 28, 2023
Merged

Auto-format code #291

merged 8 commits into from
Dec 28, 2023

Conversation

Dobiasd
Copy link
Owner

@Dobiasd Dobiasd commented Dec 26, 2023

So far, no auto-formatting was applied (at least by me), which resulted in a somewhat inconsistent code format.

@Dobiasd
Copy link
Owner Author

Dobiasd commented Dec 26, 2023

Currently, I don't have a strong opinion on the exact formatter-setting details to use. But I guess it should be something simple, so for this MR I used the default in VSCode (ms-vscode.cpptools).

@offa @pthom @tom91136 Do you see anything fundamentally wrong with this or with auto-formatting in general for this project?

@pthom
Copy link
Contributor

pthom commented Dec 27, 2023

Hello Tobias,

On my side, I do not see anything wrong with using auto-formatting. It does break the history of files (i.e. git blame will likely stop at such a commit), but in the case of fplus I think this is a minor issue.

If you are interested in this, you might want to use editorconfig. It requires a plugin with Visual Studio Code. I cannot say I have a lot of experience with it, but it would be a way to communicate the formatting choices, and let some editors take it into account.

I do think that 80 columns is a bit to low for a default (100 looks more sensible to me).

Beware, it seems that the auto-formatting went a bit too far, replacing

               view::ints(0)
                | view::take(15000000)
                | view::transform(times_3)
                | view::remove_if(is_odd_int)
                | view::transform(as_string_length)
                , 0);

by

                view::ints(0) | view::take(15000000) | view::transform(times_3) | view::remove_if(is_odd_int) | view::transform(as_string_length), 0);

(which is 135 wide)

Merry christmas!

@tom91136
Copy link
Contributor

I usually use https://clang.llvm.org/docs/ClangFormat.html which is supported by VSCode and CLion, and possibly most other IDEs. That way we can keep it all the same (as opposed to being tied to the default of the specific version of the IDE plugin) by writing a .clang-format , here's mine for example:

---
AllowShortIfStatementsOnASingleLine: Always
AllowShortCaseLabelsOnASingleLine: true
AllowShortFunctionsOnASingleLine: All
AlignEscapedNewlines: Left
IndentCaseLabels: true
ColumnLimit: 140
CompactNamespaces: true
FixNamespaceComments: true
IndentPPDirectives: BeforeHash
...

I generated mine using https://clang-format-configurator.site/

And Merry Christmas! Sorry about opening the two PRs basically on Xmas/boxing day and thanks for reviewing those.

@Dobiasd
Copy link
Owner Author

Dobiasd commented Dec 27, 2023

@pthom Oh, thanks for the remark. I did not notice this ugly format change. And, yes, git blame is a good point. 👍 But in this project, I guess I should usually be the person to blame initially anyway. 😁 Happy Holidays to you too! ❤️

@Dobiasd
Copy link
Owner Author

Dobiasd commented Dec 27, 2023

@tom91136 ClangFormat looks good. 👍 I'll play a bit with it and update this PR. And don't worry about PRs on xmas. If I would not want to respond on such days, I feel it's on me to manage my emails/notifications/todos/whatever according to my availability, same as receiving text messages during the night (of the recipient's timezone). 😉 And I enjoyed reviewing/discussing the PRs. 🤓

@Dobiasd
Copy link
Owner Author

Dobiasd commented Dec 27, 2023

For simplicity, I tried the predefined styles of clang-format (LLVM, GNU, Google, Chromium, Microsoft, Mozilla, WebKit) and visually liked WebKit the most. It also does not destroy the code example mentioned by pthom. 🎉

@offa
Copy link
Contributor

offa commented Dec 28, 2023

It might be a good idea to verify formatting as part of the CI build once merged. I can work on this if helping hands are needed. 👍

@Dobiasd Dobiasd merged commit 00a21bd into master Dec 28, 2023
37 checks passed
@Dobiasd
Copy link
Owner Author

Dobiasd commented Dec 28, 2023

@offa That sounds cool! In case it's not too complex, and you already have an idea of how to implement it, I'd be happy if you could help. ❤️

Later, I'd then maybe use whatever you implement here for frugally-deep too. 🙂

@offa offa mentioned this pull request Dec 28, 2023
@Dobiasd Dobiasd deleted the auto-format-code branch December 28, 2023 16:41
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.

4 participants