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.

Sign up for GitHub

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

Jump to bottom

Coding style for libs repo #381

Open
Andreagit97 opened this issue Jun 8, 2022 · 23 comments · May be fixed by #470
Open

Coding style for libs repo #381

Andreagit97 opened this issue Jun 8, 2022 · 23 comments · May be fixed by #470
Assignees
@Andreagit97
Labels
kind/feature New feature or request
Milestone
TBD

Comments

@Andreagit97
Copy link
Member

Andreagit97 commented Jun 8, 2022 •
edited

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 •
edited

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)

@Andreagit97 Andreagit97 linked a pull request Jul 11, 2022 that will close this issue
Open
@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

@jasondellaluce jasondellaluce mentioned this issue Dec 16, 2022
Open
@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.

@sgaist sgaist mentioned this issue Feb 2, 2024
Open
@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? 🤔

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

@Andreagit97 Andreagit97

Labels
kind/feature New feature or request
Projects
None yet
Milestone
TBD
Development

Successfully merging a pull request may close this issue.

[WIP] Introduce a coding style for libs repo Andreagit97/libs
8 participants
@leogr @FedeDP @jasondellaluce @Molter73 @deepskyblue86 @incertum @poiana @Andreagit97