NodeEditor icon indicating copy to clipboard operation
NodeEditor copied to clipboard

Code formatting rules

Open moodyhunter opened this issue 4 years ago • 8 comments

As can be seen, the code style of NodeEditor is kept manually, or sometimes, non-consistent (see https://github.com/Daguerreo/NodeEditor/commit/db2e32be1584f97c353234c5013b7247d9210109#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?

moodyhunter avatar Jul 28 '21 06:07 moodyhunter

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

IceflowRE avatar Jul 28 '21 19:07 IceflowRE

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.

moodyhunter avatar Aug 09 '21 13:08 moodyhunter

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.

Daguerreo avatar Aug 09 '21 16:08 Daguerreo

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.

moodyhunter avatar Aug 10 '21 00:08 moodyhunter

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.

paceholder avatar Aug 10 '21 20:08 paceholder

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

Daguerreo avatar Aug 11 '21 10:08 Daguerreo

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.

paceholder avatar Aug 11 '21 15:08 paceholder

Roger

Daguerreo avatar Aug 12 '21 12:08 Daguerreo