docs: Add documentation style guidelines #141

Zildj1an · 2023-10-24T13:29:03Z

Add documentation style guidelines within CONTRIBUTING.md to help clarify our expectations from a documentation standpoint. Co-developed by @00xc.

Zildj1an · 2023-10-24T13:31:43Z

Unrelated to this commit, apparently the SignedOff check can't understand multiple signatures:

From https://github.com/coconut-svsm/svsm
 * branch            main       -> FETCH_HEAD
Checking b08ee52ee93c8fdac56a31df3a1fca99d3a064b1
Author name mismatch on commit b08ee52ee93c8fdac56a31df3a1fca99d3a064b1
    Commit author name: Carlos Bilbao
    Signed-off-by name: Carlos López
Carlos Bilbao
Error: Process completed with exit code 1.

00xc · 2023-10-24T13:42:15Z

Unrelated to this commit, apparently the SignedOff check can't understand multiple signatures:

From https://github.com/coconut-svsm/svsm
 * branch            main       -> FETCH_HEAD
Checking b08ee52ee93c8fdac56a31df3a1fca99d3a064b1
Author name mismatch on commit b08ee52ee93c8fdac56a31df3a1fca99d3a064b1
    Commit author name: Carlos Bilbao
    Signed-off-by name: Carlos López
Carlos Bilbao
Error: Process completed with exit code 1.

I'll look into this

00xc · 2023-10-24T14:18:08Z

Other than the script failing, your email for the commit does not match your signed-off, could you update the PR?

Zildj1an · 2023-10-24T14:26:11Z

Other than the script failing, your email for the commit does not match your signed-off, could you update the PR?

You're absolutely correct, but, I would prefer not to close and open a new PR if the commit already has the correct signatures. Is it possible to proceed with the existing PR?

00xc · 2023-10-24T14:40:42Z

You're absolutely correct, but, I would prefer not to close and open a new PR if the commit already has the correct signatures. Is it possible to proceed with the existing PR?

Yes, you just need to amend the commit with the correct email and force push, no need to open a new PR :)

This should work:

$ git commit --author="Name <email>" --amend --no-edit && git push origin your_branch --force

00xc · 2023-10-24T14:43:19Z

In the meantime I opened #142 so this does not fail in the future.

joergroedel

Please also create a separate Documentation/ directory and put this guide into a separate file there. With the directory we can also move other general (non auto-generated) documentation files there.

CONTRIBUTING.md

00xc · 2023-10-31T12:29:25Z

Once @joergroedel's comments are addressed the new version of check-signed-off.sh should run, removing the CI failure.

Zildj1an · 2023-10-31T15:47:21Z

I have made the requested changes @joergroedel. Looks like there's still an issue with the CI @00xc?

00xc · 2023-10-31T15:47:54Z

I have made the requested changes @joergroedel. Looks like there's still an issue with the CI @00xc?

It also needs a rebase on main, my bad

Zildj1an · 2023-10-31T16:01:26Z

I have made the requested changes @joergroedel. Looks like there's still an issue with the CI @00xc?

It also needs a rebase on main, my bad

I should have known! :) It works now.

README.md

00xc

I have some additional comments. As for examples, I would perhaps have smaller more specific examples for each point that requires one.

Documentation/DOC-GUIDELINES.md

00xc · 2023-11-03T12:18:08Z

Documentation/DOC-GUIDELINES.md

+- We can't have a section "Panic" for every place the SVSM may panic, but
+  they should be included if your code checks assertions or uses the
+  `unwrap()` method. For instance:
+
+```rust
+/// # Panics
+///
+/// This function does not panic under normal circumstances. However, if
+/// the length `len` is greater than the allocated memory's actual capacity,
+/// it will panic.
+```


By the way, I just realized that running clippy in pedantic mode (cargo clippy --all -- -W clippy::all -W clippy::pedantic) will warn you about these. For example:

warning: docs for function which may panic missing `# Panics` section --> src/svsm.rs:325:1 | 325 | pub extern "C" fn svsm_start(li: &KernelLaunchInfo, vb_addr: usize) { | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | note: first possible panic found here --> src/svsm.rs:332:9 | 332 | launch_info.kernel_region_phys_start.try_into().unwrap(), | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#missing_panics_doc

I don't think we'll be running pedantic mode in our CI anytime soon, as there's just too many warnings, but maybe worth mentioning.

Add documentation style guidelines within CONTRIBUTING.md to help clarify our expectations from a documentation standpoint. Place it in a new directory Documentation/. Co-developed-by: Carlos López <[email protected]> Signed-off-by: Carlos López <[email protected]> Signed-off-by: Carlos Bilbao <[email protected]>

Zildj1an · 2023-11-09T15:14:41Z

I have some additional comments. As for examples, I would perhaps have smaller more specific examples for each point that requires one.

Thanks @00xc I applied the suggested changes.

00xc

There are still things we can cleanup, I have added more suggestions.

Documentation/DOC-GUIDELINES.md

00xc · 2023-11-09T17:29:08Z

Documentation/DOC-GUIDELINES.md

+///
+pub unsafe fn example_memcpy<T>(dest: *mut T, src: *const T, len: usize) {
+    // Ensure the pointers are not null
+    assert!(!dest.is_null() && !src.is_null());


The function should have a # Panics section in this case. We can even merge this example with the one below.

00xc · 2023-11-09T17:29:29Z

Documentation/DOC-GUIDELINES.md

+    assert!(!dest.is_null() && !src.is_null());
+    let mut rcx: usize;
+
+    unsafe {


The function is already unsafe, so there is no need for this unsafe block as far as I know.

Documentation/DOC-GUIDELINES.md

Co-authored-by: Carlos López <[email protected]>

joergroedel · 2023-11-20T14:12:30Z

Will merge it now, please send any updates on top of this.

joergroedel

LGTM

joergroedel · 2023-11-20T14:23:22Z

Merged manually. Closing.

00xc approved these changes Oct 24, 2023

View reviewed changes

Zildj1an force-pushed the doc_guidelines branch from b08ee52 to baebc8d Compare October 24, 2023 14:59

00xc approved these changes Oct 25, 2023

View reviewed changes

joergroedel requested changes Oct 27, 2023

View reviewed changes

CONTRIBUTING.md Outdated Show resolved Hide resolved

Zildj1an force-pushed the doc_guidelines branch from baebc8d to 49cd05d Compare October 31, 2023 15:44

Zildj1an force-pushed the doc_guidelines branch from 49cd05d to 2979f45 Compare October 31, 2023 16:00

00xc reviewed Oct 31, 2023

View reviewed changes

README.md Outdated Show resolved Hide resolved

Zildj1an force-pushed the doc_guidelines branch 2 times, most recently from 85459c7 to da0ea98 Compare November 1, 2023 19:38

00xc reviewed Nov 2, 2023

View reviewed changes

Documentation/DOC-GUIDELINES.md Outdated Show resolved Hide resolved

Documentation/DOC-GUIDELINES.md Outdated Show resolved Hide resolved

Documentation/DOC-GUIDELINES.md Outdated Show resolved Hide resolved

Documentation/DOC-GUIDELINES.md Outdated Show resolved Hide resolved

00xc reviewed Nov 3, 2023

View reviewed changes

Zildj1an force-pushed the doc_guidelines branch from da0ea98 to 9067a9d Compare November 9, 2023 15:14

Zildj1an closed this Nov 9, 2023

Zildj1an reopened this Nov 9, 2023

00xc suggested changes Nov 9, 2023

View reviewed changes

joergroedel and others added 2 commits November 20, 2023 15:10

Update Documentation/DOC-GUIDELINES.md

78153c0

Co-authored-by: Carlos López <[email protected]>

Apply suggestions from code review

4521395

Co-authored-by: Carlos López <[email protected]>

joergroedel approved these changes Nov 20, 2023

View reviewed changes

joergroedel closed this Nov 20, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

docs: Add documentation style guidelines #141

docs: Add documentation style guidelines #141

Zildj1an commented Oct 24, 2023

Zildj1an commented Oct 24, 2023

00xc commented Oct 24, 2023

00xc commented Oct 24, 2023

Zildj1an commented Oct 24, 2023

00xc commented Oct 24, 2023

00xc commented Oct 24, 2023

joergroedel left a comment

00xc commented Oct 31, 2023

Zildj1an commented Oct 31, 2023

00xc commented Oct 31, 2023

Zildj1an commented Oct 31, 2023

00xc left a comment

00xc Nov 3, 2023

Zildj1an commented Nov 9, 2023

00xc left a comment

00xc Nov 9, 2023

00xc Nov 9, 2023

joergroedel commented Nov 20, 2023

joergroedel left a comment

joergroedel commented Nov 20, 2023

docs: Add documentation style guidelines #141

docs: Add documentation style guidelines #141

Conversation

Zildj1an commented Oct 24, 2023

Zildj1an commented Oct 24, 2023

00xc commented Oct 24, 2023

00xc commented Oct 24, 2023

Zildj1an commented Oct 24, 2023

00xc commented Oct 24, 2023

00xc commented Oct 24, 2023

joergroedel left a comment

Choose a reason for hiding this comment

00xc commented Oct 31, 2023

Zildj1an commented Oct 31, 2023

00xc commented Oct 31, 2023

Zildj1an commented Oct 31, 2023

00xc left a comment

Choose a reason for hiding this comment

00xc Nov 3, 2023

Choose a reason for hiding this comment

Zildj1an commented Nov 9, 2023

00xc left a comment

Choose a reason for hiding this comment

00xc Nov 9, 2023

Choose a reason for hiding this comment

00xc Nov 9, 2023

Choose a reason for hiding this comment

joergroedel commented Nov 20, 2023

joergroedel left a comment

Choose a reason for hiding this comment

joergroedel commented Nov 20, 2023