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

Have more code conventions and guidelines #1394

Closed
ValuedMammal opened this issue Mar 30, 2024 · 5 comments · Fixed by #1584
Closed

Have more code conventions and guidelines #1394

ValuedMammal opened this issue Mar 30, 2024 · 5 comments · Fixed by #1584
Assignees
Labels
discussion There's still a discussion ongoing
Milestone

Comments

@ValuedMammal
Copy link
Contributor

Using rustfmt means we always agree on code formatting. If rustfmt allows it, then matters of code style can be left to personal preference.

However we could benefit from defining more code conventions in CONTRIBUTING.md for the sake of consistency. Just some ideas:

  • Forbid unsafe (or handle case by case)
  • Use StdExternalCrate style for grouping imports
  • Return as much context as possible with errors
  • Refer liberally to bitcoin BIPs and relevant historical context for a piece of code
  • Follow rust API guidelines for documentation and naming
  • Name tests after the functional unit they're testing
  • If using emoji, use them to their intended effect: code-review-emoji-guide

Approving changes:

  • All comments resolved
  • 2-ACK rule to merge

Use case
Facilitates code review and improves readability of the codebase

Additional context
#1221
#1203 (comment)

@notmandatory notmandatory added the discussion There's still a discussion ongoing label Mar 31, 2024
@notmandatory notmandatory added this to the 1.0.0-beta milestone Mar 31, 2024
@notmandatory
Copy link
Member

Great suggestions!

@ValuedMammal
Copy link
Contributor Author

Provide a reason for ignoring tests. See #1460

@oleonardolima
Copy link
Contributor

Concept ACK, thanks for the suggestions!

@storopoli
Copy link
Contributor

I really like this, I am totally on-board.
Can we discuss this in the next dev meeting? (PS: @ValuedMammal should go first since we almost never go alphabetical descending order)
Cc @nondiremanuel

@oleonardolima
Copy link
Contributor

Adding another comment, I think we should try to enforce such conventions as much as possible on CI + GitHub features so we can set and forget, as by experience on fedimint if it's not enforced by CI it gets hard to maintain such conventions/rules, and on CI I mean even as a simple semgrep if it does the job.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion There's still a discussion ongoing
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

4 participants