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

Coding style for libs repo #381

Closed
Andreagit97 opened this issue Jun 8, 2022 · 27 comments · Fixed by #2038 or #2051
Closed

Coding style for libs repo #381

Andreagit97 opened this issue Jun 8, 2022 · 27 comments · Fixed by #2038 or #2051
Assignees
Labels
kind/feature New feature or request
Milestone

Comments

@Andreagit97
Copy link
Member

Andreagit97 commented Jun 8, 2022

Motivation

Hi folks! I think we have to introduce a definitive coding style for this repository. The number of lines continuously grows and sometimes becomes difficult also to understand the code without a specific standard!
In this repo, we already have a configuration file for clang-format and VScode offers an amazing extension that allows us to format our file in a very simple way.
The rationale here is to introduce a CI/CD job that checks the coding style of every new Pull request and denies the merge until the code respects the conventions.
The initial discussion is started here but I think we need a dedicated thread for it.
It would be great to put together some ideas here!

Feature

Introduce a CI/CD job that checks the coding style of every new Pull request and denies the merge until the code respects the conventions.

@Andreagit97 Andreagit97 added the kind/feature New feature or request label Jun 8, 2022
@Andreagit97
Copy link
Member Author

cc @leogr @Molter73

@Molter73
Copy link
Contributor

Molter73 commented Jun 8, 2022

Big +1 from me, having coherent code is always nice. I also have no strong opinions on what the style should be, so the existing clang-format file looks good to me. I do feel strongly that style must be properly enforced through a CI job as stated.

Further more, in order to not force people to use vscode for formatting, we might want to consider adding some sort of make target or script that runs clang-format locally so validation before committing is easily done. As an example, this is how we handle code formatting in stackrox/collector: https://github.com/stackrox/collector/blob/b3ff69b6d5a86eba00c25a8569a79c2f77546612/collector/Makefile#L64-L74. Bonus points if we provide a pre-commit hook for checking style so contributors don't have to do the mental exercise of remembering to format the code before committing.

@Andreagit97
Copy link
Member Author

Big +1 from me, having coherent code is always nice. I also have no strong opinions on what the style should be, so the existing clang-format file looks good to me. I do feel strongly that style must be properly enforced through a CI job as stated.

Further more, in order to not force people to use vscode for formatting, we might want to consider adding some sort of make target or script that runs clang-format locally so validation before committing is easily done. As an example, this is how we handle code formatting in stackrox/collector: https://github.com/stackrox/collector/blob/b3ff69b6d5a86eba00c25a8569a79c2f77546612/collector/Makefile#L64-L74.

We absolutely need a script or a make target!

Bonus points if we provide a pre-commit hook for checking style so contributors don't have to do the mental exercise of remembering to format the code before committing.

This would be amazing in my opinion! 🚀

@deepskyblue86
Copy link
Contributor

This might be useful: https://github.com/muttleyxd/clang-tools-static-binaries

@deepskyblue86
Copy link
Contributor

I would also propose to change the current formatting to avoid tabsize 8, the code gets too wide (personal taste)

@poiana
Copy link
Contributor

poiana commented Sep 14, 2022

Issues go stale after 90d of inactivity.

Mark the issue as fresh with /remove-lifecycle stale.

Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Provide feedback via https://github.com/falcosecurity/community.

/lifecycle stale

@jasondellaluce
Copy link
Contributor

/remove-lifecycle stale

@poiana
Copy link
Contributor

poiana commented Dec 14, 2022

Issues go stale after 90d of inactivity.

Mark the issue as fresh with /remove-lifecycle stale.

Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Provide feedback via https://github.com/falcosecurity/community.

/lifecycle stale

@leogr
Copy link
Member

leogr commented Dec 14, 2022

/remove-lifecycle stale

@poiana
Copy link
Contributor

poiana commented Mar 14, 2023

Issues go stale after 90d of inactivity.

Mark the issue as fresh with /remove-lifecycle stale.

Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Provide feedback via https://github.com/falcosecurity/community.

/lifecycle stale

@Andreagit97
Copy link
Member Author

/remove-lifecycle stale

@Andreagit97 Andreagit97 self-assigned this Mar 20, 2023
@Andreagit97 Andreagit97 added this to the 0.11.0 milestone Mar 20, 2023
@FedeDP
Copy link
Contributor

FedeDP commented Apr 27, 2023

/milestone 0.12.0

@poiana poiana modified the milestones: 0.11.0, 0.12.0 Apr 27, 2023
@leogr leogr modified the milestones: 0.12.0, 0.13.0 May 3, 2023
@Andreagit97 Andreagit97 modified the milestones: 0.13.0, 0.12.0 Jun 7, 2023
@Andreagit97 Andreagit97 added this to the libs-backlog milestone Jun 7, 2023
@incertum
Copy link
Contributor

Proposing to schedule this and commit to it (also the reviewers) and then agree to a v1 we actually merge. WDYT @Andreagit97 we definitely need this?

@leogr
Copy link
Member

leogr commented Aug 24, 2023

I suggest scheduling this for immediately after we secure the Falco 0.36 release. Approximately between mid-Sept and early Oct. wdyt?

@incertum
Copy link
Contributor

I suggest scheduling this for immediately after we secure the Falco 0.36 release. Approximately between mid-Sept and early Oct. wdyt?

Sounds perfect to me!

@incertum
Copy link
Contributor

@Andreagit97 and @leogr kindly checking in on this as it's Nov 🙃 ty!

@Andreagit97
Copy link
Member Author

At the moment I have no time to work on this and moreover, we are near the 0.14.0 release, unfortunately, it is not a great moment to go on with this :(

@incertum
Copy link
Contributor

👍 Let's discuss in our next strategy meeting how we can support each other to get this over the finish line.

@poiana
Copy link
Contributor

poiana commented Feb 11, 2024

Issues go stale after 90d of inactivity.

Mark the issue as fresh with /remove-lifecycle stale.

Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Provide feedback via https://github.com/falcosecurity/community.

/lifecycle stale

@Andreagit97
Copy link
Member Author

/remove-lifecycle stale

@poiana
Copy link
Contributor

poiana commented May 13, 2024

Issues go stale after 90d of inactivity.

Mark the issue as fresh with /remove-lifecycle stale.

Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Provide feedback via https://github.com/falcosecurity/community.

/lifecycle stale

@Andreagit97
Copy link
Member Author

/remove-lifecycle stale

@leogr
Copy link
Member

leogr commented May 14, 2024

May we revamp this once 0.38 is out? 🤔

@poiana
Copy link
Contributor

poiana commented Aug 12, 2024

Issues go stale after 90d of inactivity.

Mark the issue as fresh with /remove-lifecycle stale.

Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Provide feedback via https://github.com/falcosecurity/community.

/lifecycle stale

@Andreagit97
Copy link
Member Author

/remove-lifecycle stale

@falcosecurity/libs-maintainers I will try to find some time at the end of August to address at least the formatting using clang-format! The ideal thing would be to have this as one of the latest commits before 0.18.0 to avoid issues with cherry-picks

@Andreagit97
Copy link
Member Author

let's wait until we really format all the code

@leogr
Copy link
Member

leogr commented Sep 11, 2024

🥳

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment