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

[no squash] Check for indent spaces #15002

Merged
merged 2 commits into from
Jan 21, 2025

Conversation

sfence
Copy link
Contributor

@sfence sfence commented Aug 17, 2024

Continue of #14983

This PR checks the indent in C/C++ and Lua files if somewhere spaces are used.
It ignores spaces in multiline comments.

To do

This PR is a Ready for Review

  • Fix found bad indents

How to test

Run grep commands and Python script over the test file:
test_C_file.txt

@sfence sfence marked this pull request as draft August 17, 2024 20:34
@Zughy Zughy added Maintenance Tasks to keep the codebase and related parts in order, including architectural improvements Code quality labels Aug 17, 2024
@sfence sfence force-pushed the add_indent_tab_check branch from 4056985 to 8e819ca Compare August 18, 2024 07:51
@sfence sfence marked this pull request as ready for review August 18, 2024 07:52
@cx384
Copy link
Contributor

cx384 commented Aug 21, 2024

I'm not sure about disallowing less than 4 spaces after a tab, i.e. in my opinion grep -n -P '^\s*([ ]\t|[ ]{4})' would be strict enough. Also, I think this shouldn't get too complex, a vague check should be sufficient, otherwise you could directly use a code formatter like GNU indent or similar.
But let's wait for opinions from other code devs.

src/unittest/test_noise.cpp Outdated Show resolved Hide resolved
builtin/game/voxelarea.lua Outdated Show resolved Hide resolved
@Desour
Copy link
Member

Desour commented Aug 21, 2024

There's a surprisingly high amount of space indentation.

But also half of the changes are getting rid of valid alignment. Alignment is too useful for readability to forbid imo. Which is why I'm against this PR in its current state. 👎

Something more conservative could maybe work, i.e.:

  • No tabs after any non-tab character.
  • If a line starts with k tabs, and the next line has k' < k tabs, there must be no space character before any non-whitespace character.
    (Rationale: Alignment never takes away tabs. Hence this indicates space being used for indentation.)

@sfence sfence force-pushed the add_indent_tab_check branch from 8e819ca to dabe621 Compare September 4, 2024 17:38
@sfence
Copy link
Contributor Author

sfence commented Sep 4, 2024

@cx384 I tried to catch only more than 3 spaces after the tab, but it still does not work well.

So I used the less restrictive solution mentioned by @Desour.

Some badly used spaces are not recognized now.

Do you think this solution is better?

@sfence sfence force-pushed the add_indent_tab_check branch from dabe621 to aa19202 Compare September 5, 2024 05:03
Copy link
Contributor

@cx384 cx384 left a comment

Choose a reason for hiding this comment

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

Otherwise, I'm fine with the indentions, but I haven't looked at the script yet.

src/client/content_cso.cpp Outdated Show resolved Hide resolved
src/client/game.cpp Show resolved Hide resolved
src/script/common/c_content.h Outdated Show resolved Hide resolved
src/script/common/c_converter.h Outdated Show resolved Hide resolved
@sfence sfence force-pushed the add_indent_tab_check branch from aa19202 to cff97ef Compare September 7, 2024 10:57
@Desour
Copy link
Member

Desour commented Sep 7, 2024

Could you please add some comments, so that for example one doesn't have to read through the whole preprocess script to find out what it does?

So, what this disallows now is spaces after tabs and multiline comments that start in the same line that another multiline comment ends?
Why do we need latter? And wouldn't former still break alignment after indentation?

@sfence sfence force-pushed the add_indent_tab_check branch from 3629936 to ba50c9d Compare September 7, 2024 18:17
@sfence
Copy link
Contributor Author

sfence commented Sep 7, 2024

Could you please add some comments, so that for example one doesn't have to read through the whole preprocess script to find out what it does?

I added some comments, I hope it helps.

So, what this disallows now is spaces after tabs and multiline comments that start in the same line that another multiline comment ends? Why do we need latter? And wouldn't former still break alignment after indentation?

Now spaces at the beginning of the line and spaces after tabs only if the indent is smaller than on the previous line.
In other cases, spaces after the indent is ignored.

The multiline comments part is questionable.
The idea is to detect something like this:

	{
		soem code
		/*
 wtf
  wtf
		*/
	}

Copy link
Member

@Desour Desour left a comment

Choose a reason for hiding this comment

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

Thanks!

Python script is a bit hard to follow.

Whitespace changes look mostly good.

util/ci/indent_tab_preprocess.py Outdated Show resolved Hide resolved
.github/workflows/whitespace_checks.yml Outdated Show resolved Hide resolved
.github/workflows/whitespace_checks.yml Outdated Show resolved Hide resolved
.github/workflows/whitespace_checks.yml Outdated Show resolved Hide resolved
util/ci/indent_tab_preprocess.py Outdated Show resolved Hide resolved
util/ci/indent_tab_preprocess.py Outdated Show resolved Hide resolved
util/ci/indent_tab_preprocess.py Outdated Show resolved Hide resolved
src/mapgen/treegen.cpp Show resolved Hide resolved
src/script/common/c_content.h Outdated Show resolved Hide resolved
.github/workflows/whitespace_checks.yml Outdated Show resolved Hide resolved
@sfence sfence force-pushed the add_indent_tab_check branch from ba50c9d to 074e947 Compare September 15, 2024 13:54
@sfence
Copy link
Contributor Author

sfence commented Sep 15, 2024

@Desour Everything should be done now.

@Desour
Copy link
Member

Desour commented Sep 18, 2024

(See also unresolved older comments.)

It's just a CI script, so it doesn't need to be perfect, right? I don't want to be overly pedantic.

@sfence sfence force-pushed the add_indent_tab_check branch from 291a0ea to 23df68b Compare September 18, 2024 18:20
@sfence
Copy link
Contributor Author

sfence commented Sep 18, 2024

@Desour Should be fixed now.

Copy link
Member

@Desour Desour left a comment

Choose a reason for hiding this comment

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

Looks fine from what I can tell.

One trivial thing I noticed: The python script is not marked executable. The other files in util/ci are.

Btw. add [no squash] to your PR title if you want no squashing.

@sfence sfence changed the title Check for indent spaces [no squash] Check for indent spaces Sep 19, 2024
@SmallJoker SmallJoker added the Rebase needed The PR needs to be rebased by its author label Dec 30, 2024
@Zughy
Copy link
Contributor

Zughy commented Jan 3, 2025

@sfence rebase needed

@sfence sfence force-pushed the add_indent_tab_check branch from 23df68b to 9a36104 Compare January 20, 2025 19:25
@sfence sfence force-pushed the add_indent_tab_check branch from 9a36104 to f2b0772 Compare January 20, 2025 19:48
@sfence sfence removed the Rebase needed The PR needs to be rebased by its author label Jan 20, 2025
@Desour
Copy link
Member

Desour commented Jan 20, 2025

(My approval holds. You can merge with self-approval, btw.)

@sfence sfence merged commit af3f696 into luanti-org:master Jan 21, 2025
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Code quality Maintenance Tasks to keep the codebase and related parts in order, including architectural improvements One approval ✅ ◻️
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants