Skip to content

First draft of a STYLE_GUIDE.md#526

Open
kensimon wants to merge 4 commits intoNVIDIA:mainfrom
kensimon:style-guide-first-draft
Open

First draft of a STYLE_GUIDE.md#526
kensimon wants to merge 4 commits intoNVIDIA:mainfrom
kensimon:style-guide-first-draft

Conversation

@kensimon
Copy link
Contributor

@kensimon kensimon commented Mar 11, 2026

Description

It came up that we could use a style guide to help people with things
that are commonly nit-picked in code review. And with AI-assisted
development being more common, it'd be useful to help AI write code that
adheres to our standards the first time.

So here's a first draft... this is mostly a brain dump of stuff I'm
interested in and commonly call out in code reviews. I'm hoping this
will spur discussion, since this is mostly just my opinion and others
may disagree on some of this. It's also very incomplete, and we can add
to it as we go.

It's also disorganized (there's not a lot of logic to the ordering) and
maybe a bit too example-heavy. Maybe brevity is better?

Type of Change

  • Add - New feature or capability
  • Change - Changes in existing functionality
  • Fix - Bug fixes
  • Remove - Removed features or deprecated functionality
  • Internal - Internal changes (refactoring, tests, docs, etc.)

Related Issues (Optional)

Breaking Changes

Testing

  • Unit tests added/updated
  • Integration tests added/updated
  • Manual testing performed
  • No testing required (docs, internal refactor, etc.)

Additional Notes

Rendered: https://github.com/kensimon/bare-metal-manager-core/tree/style-guide-first-draft/STYLE_GUIDE.md

It came up that we could use a style guide to help people with things
that are commonly nit-picked in code review. And with AI-assisted
development being more common, it'd be useful to help AI write code that
adheres to our standards the first time.

So here's a first draft... this is mostly a brain dump of stuff I'm
interested in and commonly call out in code reviews. I'm hoping this
will spur discussion, since this is mostly just my opinion and others
may disagree on some of this. It's also very incomplete, and we can add
to it as we go.

It's also disorganized (there's not a lot of logic to the ordering) and
maybe a bit too example-heavy. Maybe brevity is better?
@kensimon kensimon requested a review from a team as a code owner March 11, 2026 20:27
@github-actions
Copy link

🛡️ Vulnerability Scan

🚨 Found 74 vulnerability(ies)
📊 vs main: 74 (no change)

Severity Breakdown:

  • 🔴 Critical/High: 74
  • 🟡 Medium: 0
  • 🔵 Low/Info: 0

🔗 View full details in Security tab

🕐 Last updated: 2026-03-11 20:29:30 UTC | Commit: 04e9498

@github-actions
Copy link

🔐 TruffleHog Secret Scan

No secrets or credentials found!

Your code has been scanned for 700+ types of secrets and credentials. All clear! 🎉

🔗 View scan details

🕐 Last updated: 2026-03-11 20:29:34 UTC | Commit: 04e9498

Copy link
Contributor

@Matthias247 Matthias247 left a comment

Choose a reason for hiding this comment

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

Thanks @kensimon . That's a great starting point.

I think priority wise I'd probable move some of the more "service design" related sections higher up, and the in-depth coding ones (avoid mut) further down.

Maybe we can add some headlines and intra-documented links to jump quicker between these sections.

}
```

If your type has fields that can all be default values in the common case (like a Config object), prefer implementing
Copy link
Contributor

Choose a reason for hiding this comment

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

I think passing a Config object to new is also a good pattern. Doesn't necessarily need to be a builder. The core idea is to keep the amount of of arguments to the function low

STYLE_GUIDE.md Outdated
callers to pass a `PgPool` and avoid needing boilerplate to begin a transaction and commit it just to call a
read-only function.

### Metrics
Copy link
Contributor

Choose a reason for hiding this comment

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

That can probably be its own top level section (promote to ##). Same for everything below.

All the super-general "rust nitpicks" stuff is now last, in a section
"General Rust Coding Standards". Everything before it is now promoted to
'##'-level, and I did some basic rearranging by priority: Reviewability
is now first, then lints and warnings, etc.
`machine_id` are probably the two most important examples, but try to express other relevant data as fields instead of
using interpolation if it makes sense.

## Create Features
Copy link
Contributor

Choose a reason for hiding this comment

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

crate

@kensimon
Copy link
Contributor Author

I think priority wise I'd probable move some of the more "service design" related sections higher up, and the in-depth coding ones (avoid mut) further down.

Totally agree, I just pushed a commit that rearranged them:

All the super-general "rust nitpicks" stuff is now last, in a section "General Rust Coding Standards". Everything before it is now promoted to '##'-level, and I did some basic rearranging by priority: Reviewability is now first, then lints and warnings, etc.

@chet
Copy link
Contributor

chet commented Mar 11, 2026

This is exciting. I think a "Comments" section explaining how to write comments would also be useful, i.e.

/// preferred is the correct pattern for writing comments around functions,
/// with the function name as the subject.
fn preferred() {}

/// This is not the correct pattern for writing comments around functions,
/// since the name is not the subject.
fn preferred() {}

..or maybe that's NOT how we want it?

And same with structs:

struct MyPreferredStruct {
  /// preferred is how to comment on the preferred parameter,
  /// with the name of the parameter as the subject.
  pub preferred: String,
}

struct MyAvoidStruct {
  /// This is now how to comment on the preferred parameter.
  pub preferred: String,
}

..or maybe we want it the inverse? I've always done it this way, but maybe people like it the other way?

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.

3 participants