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

Adding clang format. #436

Open
wants to merge 34 commits into
base: main
Choose a base branch
from
Open

Conversation

DanielWielanek
Copy link

Adding clang format to code.

@DanielWielanek DanielWielanek added good first issue Good for newcomers infrastructure Build environment and configuration labels Nov 10, 2022
.clang-format Outdated Show resolved Hide resolved
Co-authored-by: Dmitry Kalinkin <[email protected]>
plexoos
plexoos previously approved these changes Nov 11, 2022
Copy link
Member

@plexoos plexoos left a comment

Choose a reason for hiding this comment

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

Daniel, how do you propose to enforce the style? Have you looked at any solutions?

.clang-format Outdated Show resolved Hide resolved
@plexoos
Copy link
Member

plexoos commented Nov 17, 2022

While you are testing your new workflow, please don't trigger the regular build. Maybe there is a better way to disable a workflow but you can just remove the yaml file temporarily

@plexoos plexoos dismissed their stale review November 17, 2022 20:35

not ready

@DanielWielanek
Copy link
Author

Hi, Im' still trying to test this code - but It is hard to do this on single PC because this use github stuff.
With first commint I managed to use sucesfully the format check - however this check all files - what as you can expect triggers thousands of errors. Now I'm trying to limit check to changed files only.

@plexoos
Copy link
Member

plexoos commented Dec 1, 2022

Will this action check only modified files?

@DanielWielanek
Copy link
Author

DanielWielanek commented Dec 1, 2022 via email

plexoos added a commit to plexoos/star-sw that referenced this pull request Dec 8, 2022
@plexoos
Copy link
Member

plexoos commented Dec 9, 2022

Thank you Daniel for implementing this check. I tested it in #461 and I think it works as intended. However, I wonder if you saw this action https://github.com/marketplace/actions/get-all-changed-files Looks like a straight forward solution to get a list of added or modified files in a PR

@plexoos
Copy link
Member

plexoos commented Dec 9, 2022

Moved from #445 (comment)

Actually is a funny story.
For over two weeks, I had a problem with getting only the list of modified files. The script that worked on my local computer didn't work on GitHub servers. Then I found this plugin. As far as I remember, I was not able to connect both plugins correctly (I do not remember why). However, when I removed the plugin for getting the list of modified files, surprisingly, I noticed that the older script is working now. I suppose that by accident, I enabled some flags during tests :)
So in principle, this plugin actually helped me :)

@plexoos
Copy link
Member

plexoos commented Dec 9, 2022

Here is another one :) I asked GPT to come up with a GitHub action to format C++ code and here is what I got:

Screen Shot 2022-12-09 at 5 19 59 PM

For a single attempt it is not bad at all!

@DanielWielanek
Copy link
Author

DanielWielanek commented Dec 14, 2022

@plexoos
Ok, so my question is - should be check all files when user call push request or only call this for changed files?
In case of checking everything it might take a lot of time (we have a lot of files).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers infrastructure Build environment and configuration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants