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: add style guide #1137

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

docs: add style guide #1137

wants to merge 1 commit into from

Conversation

samlaf
Copy link
Contributor

@samlaf samlaf commented Jan 22, 2025

Why are these changes needed?

Currently only added very bare bones style guide for golang.
Solidity still TODO: I'm not the best person to fill this in.

Checks

  • I've made sure the tests are passing. Note that there might be a few flaky tests, in that case, please comment that they are not relevant.
  • I've checked the new test coverage and the coverage percentage didn't drop.
  • Testing Strategy
    • Unit tests
    • Integration tests
    • This PR is not tested :(


### Comments and Documentation

- Document all exported functions, types, and variables.
Copy link
Contributor

Choose a reason for hiding this comment

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

I agree we should enforce docs for exported stuff, but I think we should also strongly encourage docs for ANYTHING non-trivial

"Does the name of this thing tell me everything I could possibly need to know?"
-> yes: ok to skip doc
-> no: write a doc, regardless of whether it's exported

- Keep functions small and focused
- Limit function length to ~50 lines where possible
- Place the most important functions at the top of the file
- Public functions that aren't methods should be placed in files with `_utils.go` suffix
Copy link
Contributor

Choose a reason for hiding this comment

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

This isn't always the case, i.e. constructors. Perhaps reword to say something like:

"Public static functions that lack a tight coupling to a specific struct should be placed in files with _utils.go suffix"

Copy link
Contributor

@cody-littley cody-littley Jan 23, 2025

Choose a reason for hiding this comment

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

Public static functions that lack a tight coupling to a specific struct (e.g. a constructor) 
should be placed in files with _utils.go suffix


### Comments and Documentation

- Document all exported functions, types, and variables.
Copy link
Contributor

@litt3 litt3 Jan 22, 2025

Choose a reason for hiding this comment

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

We should also enforce param docs for exported functions


This document outlines the coding standards and best practices for our Go projects.

### Code Organization
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider adding a section about small files, in addition to small functions

Mega files that contain multiple structs are no good. If a struct is non-trivial, it ought to have a dedicated file

@samlaf
Copy link
Contributor Author

samlaf commented Jan 27, 2025

TODO: add guide for how configs and constructors should be created (use structs, pointers to struct, how to validate config and inject defaults when not present, etc). See this discussion

@samlaf samlaf mentioned this pull request Jan 27, 2025
5 tasks
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