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

Code formatting rules #15

Open
moodyhunter opened this issue Jul 28, 2021 · 8 comments
Open

Code formatting rules #15

moodyhunter opened this issue Jul 28, 2021 · 8 comments

Comments

@moodyhunter
Copy link

As can be seen, the code style of NodeEditor is kept manually, or sometimes, non-consistent (see db2e32b#diff-0164c9b0ea36cd227809551953d8f53ea2e1f8a4d5071ff24e0778c8c4060e5eR37-R57)

This problem could be resolved by introducing a code formatting tool (e.g. clang-format), however, this may lead to some extra problems:

  1. There'll be a huge diff when initially applying the code formatting rule, causing (probably?) unnecessary git
    • As a result, merging between forks are no longer be automatic.
  2. Developers should install the formatting tool on their machine, perform formatting before committing
    • Or, do this in GitHub Actions?
@IceflowRE
Copy link

You can use a github actions to perform a check. The following is based on git-clang.

https://github.com/inexorgame/vulkan-renderer/blob/master/.github/workflows/code_style.yml

@moodyhunter
Copy link
Author

Yes indeed checking formatting in a github actions is helpful, however the major problem here is the lack of clang-format configuration file for NodeEditor.

@Daguerreo
Copy link
Owner

Daguerreo commented Aug 9, 2021

Formatting is a major issue here. I've open a PR in master repo to at least keep one code style before applying any clang-format, but has been ignored. My main concern and what stopped me to refactory the whole repository until now is that the code base will diverge with the original repo in a non-reversible way.

Github actions would be a great addition indeed, any help would be appreciated. I could start integrating that PR I mentioned before.

@moodyhunter
Copy link
Author

That's true for an actively maintained upstream, which in that case, applying the patches back to the master repo is necessary.
However, for now, it seems that the upstream @paceholder didn't respond to PRs and Issues so I think it's time to be a fresh start.

These are my thoughts, the current formatting does affect my experience as well.

@paceholder
Copy link

I'll implement che code-styling check using github actions and clang.

I used two services for continuous intergratino: travis and appveyor. Maybe they could be replaced with a github solution as well.

@Daguerreo please point me to your PR that was declined by me. I can't see it among the open ones.

@Daguerreo
Copy link
Owner

@paceholder if you please could point me out which code style should be applied I can do a much better job this time.

@paceholder
Copy link

paceholder commented Aug 11, 2021

Let's take the NodeConnectionInteraction.* from master as the baseline.
I'm not sure that clang formatter has so many tweaks to reproduce this formatting exactly, but we can try to be as close as possible.

For pointers and refs I prefer the following style:

void foo(Object const& o);
void bar(Object const* o);

Opening braces are everywhere on the new line.

Indentation --- two spaces.

Thanks.

@Daguerreo
Copy link
Owner

Roger

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

No branches or pull requests

4 participants