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

Add clang format #277

Open
PhiBabin opened this issue Sep 26, 2018 · 11 comments
Open

Add clang format #277

PhiBabin opened this issue Sep 26, 2018 · 11 comments
Assignees
Projects

Comments

@PhiBabin
Copy link
Contributor

It would be easier to submit pull request, if the indentation/format rules were enforced by an automatic tools like clang-format.

@pomerlef
Copy link
Collaborator

Sounds like a good idea.

@pomerlef pomerlef added this to Low priority in Maintenance Oct 10, 2018
@pomerlef pomerlef moved this from Pending tasks to Scheduled tasks in Maintenance Oct 10, 2018
@simonpierredeschenes
Copy link
Collaborator

I don't know if this is what we want. When we will add the formater, the first commit will contain a huge amount of lines changed, which will ruin git blame in almost all files. This must be taken into account before adding it.

@pomerlef
Copy link
Collaborator

Alright, should we move that feature in v2.0.0 then?

@simonpierredeschenes
Copy link
Collaborator

I think it would more reasonable, since history is less important between major releases.

@pomerlef pomerlef moved this from Scheduled tasks to Version 2 in Maintenance Oct 15, 2018
@stephanemagnenat
Copy link
Collaborator

A note about that. We had a similar discussion in aseba. Beside breaking the history, automatic formatting also breaks some special formatting done on purpose by the programmer (for example to define a matrix or highlight the specifics of an equation). For new code, a // clang-format off/on comment allows to disable automatic formatting, and thus it is acceptable. But for old code, I do not see the benefit. My advise is to apply it to new and changed code only.

@pomerlef
Copy link
Collaborator

pomerlef commented Oct 16, 2018

Specifications:

  • Encode our loosely defined formatting rules, see aseba rules
  • Limit automatic parsing to PR
  • Add delimiters to protect from automatic formatting, this could be // clang-format off/on

@pomerlef pomerlef added this to the Release v2.0.0 milestone Oct 16, 2018
@stephanemagnenat
Copy link
Collaborator

This file might be a useful starting point for the formatting rules, as both aseba and libpointmatcher kind of follow "my" style.

@PhiBabin
Copy link
Contributor Author

I try 2 weeks ago to create a clang-format which cover the libpointmatcher style. It seems libpointmatcher follows aseba style.
We could add a git commit hook to the project, which run clang format on the modified files when creating a commit.
I suggest to add this feature after #282 is merged, since boost::list_ofare not correctly handle by clang-format.

@YoshuaNava
Copy link
Contributor

Hi all,

Would it be possible to re-consider integrating a formatter? Both to old and new code. It would simplify debugging, reviewing pull requests, and contributing (if you have a fork), without spending too much time on style discussions and fixing syntax.

@pomerlef
Copy link
Collaborator

pomerlef commented Feb 6, 2020

I fine with the idea. Could you prepare a PR?

@YoshuaNava
Copy link
Contributor

YoshuaNava commented Apr 23, 2020

Yes. I second @PhiBabin proposal to use clang format, which is easy to run from CLI, with modern IDEs (for example, VSCode), is also used by other ASL projects, and gives quite a bit of flexibility.

I would divide the contribution in two PRs

  • The actual formatting tool, along with a small example on how to run it.
  • Formatting the repo or some files at a time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Maintenance
  
Version 2
Development

No branches or pull requests

5 participants