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

docs/contributing: Add coding conventions guide #248

Merged
merged 1 commit into from
Aug 4, 2023

Conversation

RaduNichita
Copy link
Contributor

@RaduNichita RaduNichita commented May 14, 2023

Add the coding conventions for Unikraft

@RaduNichita RaduNichita marked this pull request as draft May 14, 2023 22:26
@RaduNichita RaduNichita force-pushed the rnichita/coding-conventions branch 4 times, most recently from 05a85e9 to 1fbbc9e Compare May 14, 2023 23:35
@RaduNichita RaduNichita force-pushed the rnichita/coding-conventions branch 2 times, most recently from 5d68483 to 11a22ab Compare June 9, 2023 08:59
@RaduNichita RaduNichita marked this pull request as ready for review June 9, 2023 09:11
Copy link
Member

@StefanJum StefanJum left a comment

Choose a reason for hiding this comment

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

Thanks a lot @RaduNichita, I left some comments.
It seems there are still a few TODOs left from the document, so also check those out.

content/en/docs/contributing/coding-conventions.md Outdated Show resolved Hide resolved
content/en/docs/contributing/coding-conventions.md Outdated Show resolved Hide resolved
content/en/docs/contributing/coding-conventions.md Outdated Show resolved Hide resolved
content/en/docs/contributing/coding-conventions.md Outdated Show resolved Hide resolved
content/en/docs/contributing/coding-conventions.md Outdated Show resolved Hide resolved
content/en/docs/contributing/coding-conventions.md Outdated Show resolved Hide resolved
content/en/docs/contributing/coding-conventions.md Outdated Show resolved Hide resolved
content/en/docs/contributing/coding-conventions.md Outdated Show resolved Hide resolved
@RaduNichita RaduNichita force-pushed the rnichita/coding-conventions branch 2 times, most recently from 15ac938 to 182dbee Compare June 19, 2023 00:05
@RaduNichita RaduNichita force-pushed the rnichita/coding-conventions branch 2 times, most recently from 0f72302 to a16d556 Compare June 19, 2023 00:17
@RaduNichita RaduNichita force-pushed the rnichita/coding-conventions branch 2 times, most recently from c1d2b26 to 4322cce Compare June 26, 2023 14:35
Copy link
Member

@michpappas michpappas left a comment

Choose a reason for hiding this comment

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

Hi, this review comes unsolicited, I just wanted to look something up in our guidelines and ended up seeing things here and there. Hope it's okay :)

content/en/docs/contributing/coding-conventions.md Outdated Show resolved Hide resolved
content/en/docs/contributing/coding-conventions.md Outdated Show resolved Hide resolved
content/en/docs/contributing/coding-conventions.md Outdated Show resolved Hide resolved
content/en/docs/contributing/coding-conventions.md Outdated Show resolved Hide resolved
content/en/docs/contributing/coding-conventions.md Outdated Show resolved Hide resolved
content/en/docs/contributing/coding-conventions.md Outdated Show resolved Hide resolved
content/en/docs/contributing/coding-conventions.md Outdated Show resolved Hide resolved
The implementation is usually found in one of the libraries in `/lib` or defined in the headers in `/include/uk`.
For example: `uk_alloc()`

* `ukplat_` - denotes a public Unikraft API, whose implementation is usually provided in `/plat` and depends on the platform (e.g., KVM) for which the Unikernel is compiled.
Copy link
Member

Choose a reason for hiding this comment

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

ukplat and ukarch are subject to become obsolete by plat-rearch

Copy link
Contributor

Choose a reason for hiding this comment

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

@michpappas, could you suggest a way to rephrase this? Should we drop it altogether?

Copy link
Member

@StefanJum StefanJum left a comment

Choose a reason for hiding this comment

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

See the comments @RaduNichita.

Besides those, there seems to be a few more TODO items.
Also add dots at the end of sentences, even if in a list. I did not add comments to all of them to avoid spam, searched them with [^.]$. Also check the linter errors.

content/en/docs/contributing/coding-conventions.md Outdated Show resolved Hide resolved
content/en/docs/contributing/coding-conventions.md Outdated Show resolved Hide resolved
content/en/docs/contributing/coding-conventions.md Outdated Show resolved Hide resolved
content/en/docs/contributing/coding-conventions.md Outdated Show resolved Hide resolved
content/en/docs/contributing/coding-conventions.md Outdated Show resolved Hide resolved
content/en/docs/contributing/coding-conventions.md Outdated Show resolved Hide resolved
content/en/docs/contributing/coding-conventions.md Outdated Show resolved Hide resolved
content/en/docs/contributing/coding-conventions.md Outdated Show resolved Hide resolved
content/en/docs/contributing/coding-conventions.md Outdated Show resolved Hide resolved
@RaduNichita RaduNichita force-pushed the rnichita/coding-conventions branch 2 times, most recently from c50a27d to dc2c0a2 Compare July 19, 2023 22:28
@RaduNichita RaduNichita force-pushed the rnichita/coding-conventions branch 2 times, most recently from cdf2dfb to a9239a6 Compare July 19, 2023 22:46
Copy link
Member

@StefanJum StefanJum left a comment

Choose a reason for hiding this comment

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

Looks better @RaduNichita, thank you. Please check the unresolved conversations, if it is the case mark them as resolved, if they need more input from someone please ask for it.

content/en/docs/contributing/coding-conventions.md Outdated Show resolved Hide resolved
content/en/docs/contributing/coding-conventions.md Outdated Show resolved Hide resolved
content/en/docs/contributing/coding-conventions.md Outdated Show resolved Hide resolved
content/en/docs/contributing/coding-conventions.md Outdated Show resolved Hide resolved
content/en/docs/contributing/coding-conventions.md Outdated Show resolved Hide resolved
content/en/docs/contributing/coding-conventions.md Outdated Show resolved Hide resolved
content/en/docs/contributing/coding-conventions.md Outdated Show resolved Hide resolved
content/en/docs/contributing/coding-conventions.md Outdated Show resolved Hide resolved
Copy link
Member

@StefanJum StefanJum left a comment

Choose a reason for hiding this comment

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

Looks great @RaduNichita, see the few comments, we'll then wait for @razvand and @michpappas to have a look.

content/en/docs/contributing/coding-conventions.md Outdated Show resolved Hide resolved
content/en/docs/contributing/coding-conventions.md Outdated Show resolved Hide resolved
content/en/docs/contributing/coding-conventions.md Outdated Show resolved Hide resolved
content/en/docs/contributing/coding-conventions.md Outdated Show resolved Hide resolved
Copy link
Member

@StefanJum StefanJum left a comment

Choose a reason for hiding this comment

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

Looks fine to me @RaduNichita, thanks a lot 🥇

@razvand razvand force-pushed the rnichita/coding-conventions branch 2 times, most recently from d8b91d0 to 27ac3f2 Compare August 4, 2023 19:09
Add extensive conventions for writing code. The goal is to create a
consistent set of rules & guidelines to be used throughout the Unikraft
codebase.

Signed-off-by: Radu Nichita <radunichita99@gmail.com>
@razvand razvand force-pushed the rnichita/coding-conventions branch from 27ac3f2 to e893e48 Compare August 4, 2023 19:12
Copy link
Contributor

@razvand razvand left a comment

Choose a reason for hiding this comment

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

Thanks, @RaduNichita.

Reviewed-by: Razvan Deaconescu razvand@unikraft.io
Approved-by: Razvan Deaconescu razvand@unikraft.io

@razvand razvand merged commit f7da0bd into unikraft:main Aug 4, 2023
1 check passed
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.

None yet

4 participants