Daguerreo / NodeEditor

Qt Node Editor. Dataflow programming framework
BSD 3-Clause "New" or "Revised" License
18 stars 5 forks source link

Code formatting rules #15

Open moodyhunter opened 3 years ago

moodyhunter commented 3 years ago

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?
IceflowRE commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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

paceholder commented 3 years ago

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 commented 3 years ago

Roger