GodotVR / godot_oculus_mobile

Godot Oculus mobile drivers (Oculus Go / Oculus Quest)
MIT License
169 stars 34 forks source link

Cleanup project indentation and formatting #109

Closed m4gr3d closed 3 years ago

m4gr3d commented 4 years ago

@BastiaanOlij @NeoSpark314 This is discussion PR to decide on a formatting and indentation style for the project.

The plan is to use the .editorconfig configuration file to define the style since it's easy to edit and is widely supported. Once we decide on the style, I'll make a pass through the rest of the project, apply the new style, and push the changes in this PR.

BastiaanOlij commented 4 years ago

Ah, I've never setup CI for these projects. There is actually a clang-format file in the root that applies all the formatting rules also used for Godot and in the hooks folder are hooks you can copy into your .git folder so the formatting is automatically checked on committing changes: https://github.com/GodotVR/godot_oculus_mobile/tree/master/hooks

Is there a good reason to not use the existing logic?

m4gr3d commented 4 years ago

Ah, I've never setup CI for these projects. There is actually a clang-format file in the root that applies all the formatting rules also used for Godot and in the hooks folder are hooks you can copy into your .git folder so the formatting is automatically checked on committing changes: https://github.com/GodotVR/godot_oculus_mobile/tree/master/hooks

Is there a good reason to not use the existing logic?

@BastiaanOlij Is there a way to have clang-format pick up formatting from the .editorconfig file? The Godot project also uses an .editorconfig file, so I thought they were interoperable, and .editorconfig is picked up automatically by Android Studio, which provides editor support.

BastiaanOlij commented 4 years ago

@BastiaanOlij Is there a way to have clang-format pick up formatting from the .editorconfig file? The Godot project also uses an .editorconfig file, so I thought they were interoperable, and .editorconfig is picked up automatically by Android Studio, which provides editor support.

@m4gr3d that seems counter productive to me, the reason for using the clang-format file from the main Godot repo is to ensure we follow the same formatting rules as Godot. I wasn't aware the main Godot repo also contains an .editorconfig, I'm guessing that must have been added so people doing work on the Android version of Godot didn't have to constantly fix up formatting issues. @akien-mga would know :)

To be honest, it's just a starting point and it may prove valuable if we do enable CI checking with clang-format so we prevent PRs being merged that do not follow the style guidelines (unless that is also possible with ,editorconfig), otherwise seeing this is an Android only plugin it makes sense to just switch to .editorconfig.

akien-mga commented 4 years ago

.editorconfig is an extra hint for those IDEs that use it, but .clang-format is the enforced reference and if there's any conflict between the two, .editorconfig should be changed to fit the .clang-format style.

m4gr3d commented 4 years ago

@BastiaanOlij I believe .editorconfig supports multiple other IDEs and languages, and is not limited to Android projects.

How difficult would it be to enable CI checking? If we do enable it, then it'd be valuable to keep the clang-format configuration as well, and I can update it to match based on what we decide.

The goal of this PR is primarily to agree on a set of formatting rules. Once we agree on them, then I can update the relevant configuration files to match.

I think for the most part, what we have now is good. Just one area of clarification; Godot uses tab for native code, and spaces for java code. I'd like to depart from this and use spaces for both native and java code, while keeping the use of tab for gdscript code since that's the default for the Godot editor.

BastiaanOlij commented 4 years ago

How difficult would it be to enable CI checking?

I've been putting off figuring that out for waaaaaaaaaay too long. I'm going to start diving into that and start setting them up. I might start with OpenVR and see if I can also do build tests, then we can have a chat on how we can do the same for android.

I think for the most part, what we have now is good. Just one area of clarification; Godot uses tab for native code, and spaces for java code. I'd like to depart from this and use spaces for both native and java code, while keeping the use of tab for gdscript code since that's the default for the Godot editor.

Agreed, I'm fine either way, as long as we're consistent about it.

BastiaanOlij commented 4 years ago

Ok, I've enabled travis for godot_openvr , godot_oculus and godot_oculus_mobile

I'm playing around with travis for godot_openvr, it's a simple matter of creating a `.travis.yml' file and configuring stuff. If you look at the one in the main godot repo there are some hints as to setting it up to do an android build but seeing we're using ninja and such here I'm guessing only part of that is usable.

Have a look at: https://github.com/GodotVR/godot_openvr/pull/94

You could just add the bits that do the clang-format check here. Keep in mind that the script file needs to be edited too so it excludes the correct folders, it actually figures out which files have changed in a PR and only runs clang-format on those files if appropriate (stole this from the Godot version :) )

m4gr3d commented 4 years ago

@BastiaanOlij Nice! Is this the one for the godot_oculus_mobile? Do I need some kind of access?

I'll take a swing at setting up the .travis.yml config file and whatever is left this weekend.

BastiaanOlij commented 4 years ago

@m4gr3d you don't need access, as soon as you add a .travis.yml file it'll start running, it's all setup.

BastiaanOlij commented 4 years ago

@m4gr3d I think you can pretty much copy this, I've commented out the actual build code: https://github.com/GodotVR/godot_oculus/pull/28

You do need to change the paths that the grep command filters out to what suits your needs. It's designed to only run clang-format on files actually changed in the commit so you don't end up fixing someone elses issues.