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

Improving clang formatting #203

Merged
merged 7 commits into from
May 11, 2022
Merged

Improving clang formatting #203

merged 7 commits into from
May 11, 2022

Conversation

juanangp
Copy link
Member

@juanangp juanangp commented May 10, 2022

juanangp Ok: 87

Script reformat-clang has been improved:

  • Added readlink to make the script independent from the path where is launched
  • Adding support for single files including file extensions *.h, *.cxx, *.cc and *.C
  • Now the script check if the file or directory exists
  • Removing un-necessary commands

Added githook in the cmake, so the clang-format is performed when git commit is issued

  • githook is created when cmake is issued and if there is no pre-commit present.
  • Check if clang-format is present in the system, otherwise is skipped.
  • Can be used as template to add different githook features.

@juanangp juanangp requested review from jgalan, lobis and nkx111 May 10, 2022 08:37
Copy link
Member

@jgalan jgalan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I commented to @lobis it would be interesting that our Cmake system forces that reformat-clang.sh is executed at the beginning of each make execution. So that any user that launches code compilation keeps a good formatting.

@lobis
Copy link
Member

lobis commented May 10, 2022

As I commented to @lobis it would be interesting that our Cmake system forces that reformat-clang.sh is executed at the beginning of each make execution. So that any user that launches code compilation keeps a good formatting.

It could be nice but maybe it would give problems, also it takes a fair amount of time to execute the clang-format. It would be annoying for users such as me that have this done automatically on file save. I think it may be better a step on the pipeline that gives error if the source files have not been clang-formatted? (calculating the diff etc.).

What do you think @jgalan @juanangp ?

@juanangp
Copy link
Member Author

juanangp commented May 10, 2022

What do you think @jgalan @juanangp ?

Perhaps would be better to use a pre-commit hook https://stackoverflow.com/questions/55965712/how-do-i-add-clang-formatting-to-pre-commit-hook
But I am not sure how to implement it in the repo...

@lobis
Copy link
Member

lobis commented May 10, 2022

What do you think @jgalan @juanangp ?
Perhaps would be better to use a pre-commit hook https://stackoverflow.com/questions/55965712/how-do-i-add-clang-formatting-to-pre-commit-hook
But I am not sure how to implement it in the repo...

It is not possible to implement it in the repo, at most you can have a file in the repo and direct users to copy this file into their .git folder.

@juanangp
Copy link
Member Author

juanangp commented May 10, 2022

It is not possible to implement it in the repo, at most you can have a file in the repo and direct users to copy this file into their .git folder.

Can this be done at the build or in the cmake? Or this is an overkill? Also note that you need clang-format installed in your OS

@juanangp
Copy link
Member Author

I have added a githook in the cmake, so the clang-format is performed when git commit is issued.

Please, check again.

@juanangp juanangp requested review from jgalan and lobis May 10, 2022 15:03
@juanangp juanangp changed the title Improving reformat-clang script Improving clang formatting May 10, 2022
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

Successfully merging this pull request may close these issues.

3 participants