Issue #28 resolved

Use clang-format style file to enforce a coding style convention

geisserf
created an issue

We decided to use clang-format to enforce a certain code style for PROST. After this issue is completed we should have:

  • Removed the uncrustify config file
  • Inserted the .clang-format config file in /src
  • Some way of enforcing the coding style before a push is accepted

The first two points are trivial, attached to the issue is the clang-format style file.

The third point is one worth of more discussion:

  • How do we want to reject pushes?
  • Do we only check for the files which are changed? (This would allow us to close this issue before we converted everything into the new style. Old files will then get updated, whenever someone modifies them.)
  • Do we want a script that modifies commited files by itself before the push, or that tells the contributor which lines of code break the code style and require him to fix it (or to manually call a script)

This issue is related to issue #24.

Comments (7)

  1. tkeller repo owner

    If this one is a separate issue now, what do we do with the changes related to clang-format that have been done in issue-24? (Is that only revision 165)? Do we replace issue-24 completely with smaller issues or do we keep some parts in there?

  2. tkeller repo owner

    My opinion is the following:

    • How do we want to reject pushes?
    • Do we want a script that modifies commited files by itself before the push, or that tells the contributor which lines of code break the code style and require him to fix it (or to manually call a script)

    I would like to enforce the reviewing process we have started at some point from now on, i.e. all code is reviewed by someone before it gets pushed. Both the reviewer and the person writing the code should make sure that the coding style convention is met. However, if there is a possibility to have an automated script that doesn't induce a complicated repo setup, I would be fine with that as well.

    • Do we only check for the files which are changed? (This would allow us to close this issue before we converted everything into the new style. Old files will then get updated, whenever someone modifies them.)

    I'd prefer to convert everything we have once and "start" with a consistent code base. However, I would not rely on the automatic tool but actually go over the code and clean it up. E.g., many variable/class/method names are unnecessarily long for a strict 80 character line length, so we have to go over the code and rename stuff to obtain nice looking code.

  3. geisserf reporter

    If this one is a separate issue now, what do we do with the changes related to clang-format that have been done in issue-24?

    Which changes do we have? We set up the clang-format style file and applied it to all files right? The main reason for this issue is that I currently have to copy the clang-format style manually, whenever I create a new branch, since it is not contained in the main branch.

    Do we replace issue-24 completely with smaller issues or do we keep some parts in there?

    That's a good question. Maybe we could use that repo to implement changes which are more large scale, where we know we may require multiple iterations until we are satisfied?

    However, if there is a possibility to have an automated script that doesn't induce a complicated repo setup, I would be fine with that as well.

    I think it should be possible to create some hookup that applies a script for every commit. I will look into that. In general, I think we should have at least some automated way to see that a file does not conform the coding style.

    E.g., many variable/class/method names are unnecessarily long for a strict 80 character line length, so we have to go over the code and rename stuff to obtain nice looking code.

    I agree, that sounds reasonable.

  4. tkeller repo owner

    If you have a look here there are 10 commits within the issue-24 branch before the randomness branch starts. I would actually like to keep them, but I can also "move" them here to issue #28.

  5. geisserf reporter

    Ah, I see. I think we should use issue #28 for formatting only. So the question is how we apply these changes to this repo, such that they are incorporated for new branches, right? Maybe move them to issue #24 and merge them into the master branch, without closing issue #24?

    If we switch completely to Bitbucket we should think about the more general question on what commits we want to have on the master branch. Maybe we should discuss this next week to more extent.

  6. tkeller repo owner

    We decided to "blindly" run clang-format on all files even though this means that some parts of the code are uglier than before.

  7. tkeller repo owner

    This issue was resolved with bd3c1c0. Making the code nicer again (i.e., fixing all parts where clang-format made the code ugly) is part of issue #24 or will be done whenever we alter some part of the code.

  8. Log in to comment