OoT3D_Randomizer icon indicating copy to clipboard operation
OoT3D_Randomizer copied to clipboard

Cleanup formatting with clang-format.

Open PhlexPlexico opened this issue 3 years ago • 6 comments

This PR introduces a new workflow on the development and main branches, which will auto-format and commit the code if it fails to meet the specific clang-formatting. This is to ensure we keep code clean, and developers don't have to install clang-format if they choose not to (even though it is highly recommended).

I've also gone ahead and ran the code formatting once as well, hence the large amounts of files changed. I can always back this out if needed as well!

The .clang-format config was taken from zeldaret's MM decomp project as well, to ensure parity between Zelda projects.

I'm also leaving this one in draft until we have most PRs closed, to ensure we don't interrupt anyone's workflows when this goes in because it will surely cause merge conflicts.

PhlexPlexico avatar Oct 05 '22 18:10 PhlexPlexico

1: I think it should be turned off for (or adjusted for) the following:

  • Settings
  • Item locations
  • Item list
  • Descriptions
  • The seqTypesSFX list in sound_effects.cpp

Maybe for the following:

  • Location access
  • Hints

2: Other suggestions:

  • Remove column limit
  • Allow the previous of this: image
  • Add IndentPPDirectives: BeforeHash
  • Maybe add AlignConsecutiveAssignments? It's style seems to be used a lot.

Kewlan avatar Oct 05 '22 19:10 Kewlan

Settings

Yeah I can see this one being an issue. Readability isn't that great with clang-format messing around with all of it. I think there may be a way to adjust this without having a bunch of // clang-format off littered around the options. I'll continue to look into it!

Item Location

I'm a bit impartial about this one, personally, mainly due to the fact in it's current state it's not that readable since you have to scroll in order to see the rest of values, on 1920x1080 (or low PPI screens) it's even worse to go through. However, that is something we probably could ignore if this is preferred overall because I can also see the spacing being an issue.

Item List

Same here, a bit impartial about having to scroll, but this one does look a bit cleaner at least/easier to navigate.

Descriptions

Ah yes, this one should have an option to keep everything aligned with the text and = sign... I think that file could be completely ignored since it does already have its own styling.

SeqTypesSFX

Hmm, I guess my question for this is why are the comments prepended where all other comments are either above or appended in the array? As an example, see cosmetics.cpp in tunicColors.

Location Access

Again with column limits. I'm sure there can be an in-between solution for this :) even with AlignConsecutiveAssignments it still has some issues!

Hints

If it's mainly the hintSettingTable, so long as they can be kept consistent as well.

Col. Limit

I'm against removing this overall, as I feel this is one of the larger style choices that are broken most often, and this styling is meant to make it consistent across projects. However, I'm willing to part ways with this one as well if that's a general consensus too! 😄

IndentPPDirectives: BeforeHash

LGTM 👍

AlignConsecutiveAssignments

👍

PhlexPlexico avatar Oct 05 '22 20:10 PhlexPlexico

SeqTypesSFX

Hmm, I guess my question for this is why are the comments prepended where all other comments are either above or appended in the array? As an example, see cosmetics.cpp in tunicColors.

They just happen to be prepended because I copied the style from Ura's BGM shuffle implementation. (Which btw has a similar list, seqTypesMusic in music.cpp, that should be in the same style.) Whether they're before or after doesn't really matter, it's the alignment I wanna keep. Maybe possible with clang?

Kewlan avatar Oct 05 '22 20:10 Kewlan

Hmm, perhaps its possible here, if not we can simply set it to ignore as well, especially if it's just specific lists, too! I'll see what I can do!

PhlexPlexico avatar Oct 05 '22 21:10 PhlexPlexico

Is this PR waiting on anything else besides conflict resolution?

gamestabled avatar Nov 03 '22 13:11 gamestabled

I don't believe so, I think everything was acknowledged in the discord. I can rebase and fix the conflicts here shortly.

PhlexPlexico avatar Nov 03 '22 14:11 PhlexPlexico