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

use tabs for only indentation, not alignment #556

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

ASpoonPlaysGames
Copy link
Contributor

Because that's the point of using tabs...

@ASpoonPlaysGames ASpoonPlaysGames added the needs code review Changes from PR still need to be reviewed in code label Sep 16, 2023
@GeckoEidechse
Copy link
Member

GeckoEidechse commented Sep 16, 2023

So applying clang-format locally right now with this PR and not sure if stuff like this is intended...

image

Like shouldn't last indentation still be tab in this case?

EDIT: Version with clang-format applied: https://github.com/R2Northstar/NorthstarLauncher/tree/indentation-test

@ASpoonPlaysGames
Copy link
Contributor Author

Like shouldn't last indentation still be tab in this case?

That would probably depend on our settings for indenting stuff like that?

@ASpoonPlaysGames
Copy link
Contributor Author

Like shouldn't last indentation still be tab in this case?

Looked into it and no, this is considered alignment because it is handled by AlignAfterOpenBracket which we have set to AlwaysBreak

Personally, I think setting it to Align makes more sense. This results in:
image
whilst also not causing (other values of UseTab)
image

@ASpoonPlaysGames
Copy link
Contributor Author

Disadvantage of the above is stuff like:
image

@ASpoonPlaysGames
Copy link
Contributor Author

I think the current change is good enough to warrant a merge atm, it's definitely better than before

@GeckoEidechse thoughts?

@GeckoEidechse
Copy link
Member

Uh, I'll have to give it another. Haven't gotten around still. Too much to do. In general tabs should only be used for indentation cause that's how tab indentation works.

Just wanna double check how much stuff goes weird with the change.

@GeckoEidechse
Copy link
Member

Ok so looking at this again, I found in testing that vscode for example does not support this type of mixed indentation.

Not gonna lie, I'm starting to just get frustrated with tab based indentation. Like the whole point about tabs is that you can adjust indentation level to your liking, yet at the same time, you still need spaces for alignment.
Mixing tabs and spaces for alignment breaks this idea cause elements get misaligned.
But using spaces for alignment and tabs for indentation is not properly supported by IDEs cause they would have to understand the underlying language to be able to to tell whether the action we perform is alignment or indentation.

So merging this will result in people using tabs for alignment cause that's what their IDE inserts and then having CI formatting check fail with no obvious indication cause whitespace changes are invisible by definition...

 

Honestly, can we just switch everything to spaces and be done with this bs? Like as long as we have tabs, we will either have misaligned alignment for anything but tab size 4 or constant formatting issues in new PRs. There's no way around it.

@ASpoonPlaysGames
Copy link
Contributor Author

vscode for example does not support this type of mixed indentation

Why would you use vscode for launcher stuff wtf

@ASpoonPlaysGames
Copy link
Contributor Author

But using spaces for alignment and tabs for indentation is not properly supported by IDEs cause they would have to understand the underlying language to be able to to tell whether the action we perform is alignment or indentation.

Except any good IDE will just apply clang format automatically when it detects a clang format file

@ASpoonPlaysGames
Copy link
Contributor Author

Honestly, can we just switch everything to spaces and be done with this bs? Like as long as we have tabs, we will either have misaligned alignment for anything but tab size 4 or constant formatting issues in new PRs. There's no way around it.

No, accessibility is important. The way around it is to not use shit IDEs that dont use clang format

@GeckoEidechse
Copy link
Member

GeckoEidechse commented Oct 23, 2023

vscode for example does not support this type of mixed indentation

Why would you use vscode for launcher stuff wtf

Cause I'm on Linux and last time I checked visual studio was not supported on Linux :P

But using spaces for alignment and tabs for indentation is not properly supported by IDEs cause they would have to understand the underlying language to be able to to tell whether the action we perform is alignment or indentation.

Except any good IDE will just apply clang format automatically when it detects a clang format file

Visual Studio exhibits the exact same behaviour as vscode btw. Keep in mind that both pickup clang-format and can select to apply for formatting but when doing normal editing, both fail to switch from "indentation mode" to "alignment mode" during normal editing.

So for example in this screenshot, assuming the right line is our cursor

image

hitting the TAB key in both IDEs will enter a TAB character even though we are in alignment mode right now and it should've converted the entered tab to 4 spaces.

 

Honestly, can we just switch everything to spaces and be done with this bs? Like as long as we have tabs, we will either have misaligned alignment for anything but tab size 4 or constant formatting issues in new PRs. There's no way around it.

No, accessibility is important. The way around it is to not use shit IDEs that dont use clang format

How is tabs vs spaces and accessibility thing? Is it in regards to smaller screen sizes?

@ASpoonPlaysGames
Copy link
Contributor Author

hitting the TAB key in both IDEs will enter a TAB character even though we are in alignment mode right now and it should've converted the entered tab to 4 spaces.

Iirc clang format isnt applied after each keystroke, because that would be absurd. I have mine set to format a line when i finish the line, as well as on file save. I forget what the default is

@ASpoonPlaysGames
Copy link
Contributor Author

Cause I'm on Linux and last time I checked visual studio was not supported on Linux :P

There are other IDEs. Vscode isnt an IDE, its a text editor that people give some IDE features through extensions

@ASpoonPlaysGames
Copy link
Contributor Author

How is tabs vs spaces and accessibility thing? Is it in regards to smaller screen sizes?

Can be. I've seen people who have problems determining indentation differences on lower widths as well, so they run 6-8 width

@ASpoonPlaysGames
Copy link
Contributor Author

Honestly the real solution here is to make clang format apply on file save. Not sure if you can define that in like .editorconfig or something

@GeckoEidechse
Copy link
Member

hitting the TAB key in both IDEs will enter a TAB character even though we are in alignment mode right now and it should've converted the entered tab to 4 spaces.

Iirc clang format isnt applied after each keystroke, because that would be absurd.

Obviously.

My complaint is that IDEs (editors, whatever) don't properly switch between tabs and spaces when pressing the TAB key when switching between indentation and alignment.

In an all-spaces codebase, the editor properly converts a TAB press to spaces.
In a codebase that uses tabs for indentation, the editor converts a TAB press to tabs. So when you want to do alignment, you either have to manually press space bar a bunch of times or resort to auto-formatter (which based on the amount of PRs we have with format errors, not everyone does :P)

@ASpoonPlaysGames
Copy link
Contributor Author

Perhaps we could just not have format check, but instead just make github actions commit a formatted version or something

@ASpoonPlaysGames
Copy link
Contributor Author

@GeckoEidechse Can you update that branch you have and take a look? Hopefully it should be better now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs code review Changes from PR still need to be reviewed in code
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

3 participants