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

[Merged by Bors] - Add CODING.md #6045

Closed
wants to merge 3 commits into from
Closed

[Merged by Bors] - Add CODING.md #6045

wants to merge 3 commits into from

Conversation

acud
Copy link
Contributor

@acud acud commented Jun 14, 2024

Motivation

As a part of an effort to standardize the way we develop software I am suggesting this as an entry point to a discussion about how to write and expect code to be checked into the codebase.

Description

While a lot of the suggestions here don't match what is actually in the codebase, it is meant to describe the point we aspire to get to, and slowly mutate the codebase into that direction by altering what is already there or by the style in which new components are written. It is meant as a departure point into a discussion about the content, rather than an attempt to coerce the content as something set in stone, therefore, suggestions and questions are welcome.

Test Plan

Not to achieve a quorum but to see if anyone in the team objects to this format and content.

@acud acud self-assigned this Jun 14, 2024
Copy link
Member

@fasmat fasmat left a comment

Choose a reason for hiding this comment

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

Thanks for providing a great starting point for this! 🙂

Overall I agree with the suggested coding style. I added a few comments regarding details that I'm unsure about or feel like could be added to the document.

CODING.md Show resolved Hide resolved
Comment on lines +27 to +28
Errors must be propagated by the caller function. Error should not be logged and passed at the same time. It is up to the next caller function to decide what should be done in case of an error (e.g. execute specific logic, log, or continue bubbling the error up the stack).

Copy link
Member

@fasmat fasmat Jun 17, 2024

Choose a reason for hiding this comment

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

Should we add to use the general pattern of using switch and errors.Is/errors.As like:

var customError MyErroType
err := someFunctionCall()
switch {
case errors.Is(err, Error1):
   // handle Error1
case errors.As(err, &customError):
   // handle customError
case err != nil: // another error
   // handle generic error
}

or leave that up to the developer?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that using errors.Is/As should be encouraged yes. About using switches - I'm not 100% sure because it's a general deviation from the usual if err := ... {} structure and switches have the problematic aspect of when they have the default: case it would handle the happy path which is a bit misleading. Ideally, the happy path should never be nested, but rather handled at the base nesting level of the method (only error paths should be nested). It is indeed sometimes unavoidable to handle happy path this way but switches may cause this to happen more often than not.

Also as a sidenode it should probably also be noted that happy path shouldn't be nested :)

I'd like to cover all those in a separate styleguide that would have examples too.

Copy link
Contributor

Choose a reason for hiding this comment

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

Using switch doesn't mean that the happy path must be nested. The errors can be handled in the switch cases and the happy path - below:

err := someFunctionCall()
switch {
case errors.Is(err, Error1):
   // handle Error1
case err != nil:
   // handle unknown error
}

// happy path goes here

CODING.md Outdated Show resolved Hide resolved
CODING.md Show resolved Hide resolved
@acud acud requested review from fasmat and ivan4th June 20, 2024 21:57
@acud
Copy link
Contributor Author

acud commented Jun 20, 2024

I've addressed most of the comments here, please see the diffs that I've pushed. I suggest to address specific style-guide points in a separate document that will be more detailed.

Copy link
Member

@fasmat fasmat left a comment

Choose a reason for hiding this comment

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

lgtm, if @ivan4th and @poszu have no objections, I'd say its ready to be merged and go into effect 🙂

@acud
Copy link
Contributor Author

acud commented Jun 24, 2024

bors merge

spacemesh-bors bot pushed a commit that referenced this pull request Jun 24, 2024
## Motivation

As a part of an effort to standardize the way we develop software I am suggesting this as an entry point to a discussion about how to write and expect code to be checked into the codebase.
<!-- Give a brief description of the motivation for this PR. 1-2 sentences is fine. -->
@spacemesh-bors
Copy link

Pull request successfully merged into develop.

Build succeeded:

@spacemesh-bors spacemesh-bors bot changed the title Add CODING.md [Merged by Bors] - Add CODING.md Jun 24, 2024
@spacemesh-bors spacemesh-bors bot closed this Jun 24, 2024
@spacemesh-bors spacemesh-bors bot deleted the coding-md branch June 24, 2024 13:41
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