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

Reformat the code #248

Closed
wants to merge 3 commits into from
Closed

Reformat the code #248

wants to merge 3 commits into from

Conversation

numberZero
Copy link
Contributor

@numberZero numberZero commented Oct 4, 2023

This is a (hopefully final) prerequisite for minetest/minetest#13642.

I used .clang-format from Minetest but altered it a bit. In particular, clang-format isn’t all that good on wrapping long lines, so I removed line length limit to avoid ugly continuation.

Now, I don’t claim it is all pretty and nice now, but it’s much more consistent with both Minetest and itself.

@numberZero
Copy link
Contributor Author

Reapplied.

@numberZero
Copy link
Contributor Author

Reapplied. @Desour

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.

Does what it says (apart from those 2 spaces).

How hard would it be to replace the "Strip non-leading tabs" commit with one that replaces all the tabs with the right amount of spaces? It's a bit sad that we're loosing the alignment.

source/Irrlicht/CGLXManager.cpp Show resolved Hide resolved
source/Irrlicht/CGLXManager.cpp Show resolved Hide resolved
@numberZero
Copy link
Contributor Author

It's a bit sad that we're loosing the alignment.

Alignment?
Oh wait, you’re likely still using a monospaced font. Okay...

How hard would it be to replace the "Strip non-leading tabs" commit with one that replaces all the tabs with the right amount of spaces?

Doing exactly what you suggest is possible but won’t help as great deal of these non-leading tabs wasn’t even legitimate alignment. Up to this sample in include/SMaterialLayer.h. Manually working through that is possible (it’s just 1k lines) but out of scope of this PR.

Saying that, now I think that commit wasn’t at all good, due to e.g. code samples in comments. I removed it from the branch as it would be much easier to reapply its good part than to undo its bad part later.

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.

LGTM.

@Desour
Copy link
Member

Desour commented Dec 13, 2023

Oh wait, you’re likely still using a monospaced font. Okay...

Yes, I just find it's neat.

Manually working through that is possible (it’s just 1k lines) but out of scope of this PR.

I thought more about writing a script that does it.

Saying that, now I think that commit wasn’t at all good, due to e.g. code samples in comments. I removed it from the branch as it would be much easier to reapply its good part than to undo its bad part later.

Good solution for now. 👍

find -type f |  # list all regular files
  grep -E '\.(h|cpp|mm)$' |  # filter for source files
  grep -v '/mt_' |  # filter out generated files
  xargs -n 1 -P $(nproc) clang-format -i  # reformat everything
@numberZero
Copy link
Contributor Author

Reapplied.
Yes, it has to be reapplied when pretty much any other PR is merged. For the obvious reason. Maybe it’s better to split it in parts?

@sfan5
Copy link
Member

sfan5 commented Dec 17, 2023

I think we should do this as the very last step before importing it into the MT repo since at that point the ability to easily backport upstream commits and all PRs on this repo are lost anyway.

@numberZero
Copy link
Contributor Author

What else holds the import, then?

@Desour
Copy link
Member

Desour commented Jan 21, 2024

It has been decided in the last dev meeting, that #276 will be merged first. After that one is merged, we won't trigger another rebase of this PR.
See discussion starting here: https://irc.minetest.net/minetest-dev/2024-01-21#i_6147328

@sfan5
Copy link
Member

sfan5 commented Feb 23, 2024

I think we can also use this opportunity to move all files from source/Irrlicht to just src. thoughts?

@numberZero numberZero closed this by deleting the head repository Feb 24, 2024
@Desour
Copy link
Member

Desour commented Mar 3, 2024

@numberZero I've seen you've deleted some of your fork repos. If you don't want to work on this anymore, we'll adopt this. :-) I hope you're doing fine though.

@Desour
Copy link
Member

Desour commented Mar 20, 2024

#295

I think we can also use this opportunity to move all files from source/Irrlicht to just src. thoughts?

Included in new PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants