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

Wrap errors from newest to oldest #105

Closed
2 tasks done
c-pius opened this issue Nov 21, 2024 · 2 comments · Fixed by #112
Closed
2 tasks done

Wrap errors from newest to oldest #105

c-pius opened this issue Nov 21, 2024 · 2 comments · Fixed by #112
Assignees
Labels

Comments

@c-pius
Copy link
Contributor

c-pius commented Nov 21, 2024

Description

Sometimes we wrap errors like <older>: <newer>, e.g., in: https://github.com/kyma-project/modulectl/blob/main/internal/service/create/create.go#L222

We should rather wrap them in the different order <newer>: <older>

Also take care of reformulating err message then if the order changes to make more sense.

Reasons

See Google Style guide Placement of %w in errors. It is good practice to wrap like <newer>: <older> so that the printed error reads from newer to older (i.e., generic to specific) and so that the print is aligned with the unwrapping of the error.

AC

  • investigated whether possible to configure a linter catching this, if so, configured accordingly
    • check if the linter is maybe disabled in the current config
  • all occurrences in the code fixed

Attachments

@medmes medmes self-assigned this Nov 21, 2024
medmes added a commit to medmes/modulectl that referenced this issue Nov 22, 2024
medmes added a commit to medmes/modulectl that referenced this issue Nov 25, 2024
@lindnerby lindnerby linked a pull request Nov 26, 2024 that will close this issue
@medmes
Copy link
Member

medmes commented Nov 26, 2024

After some investigation regarding the linter, the errorlint and wrapcheck are already enabled by default except the test files. those linters already check if the error are already wrapped. I suggest, in the next PRs that we keep an eye on error handling and avoid the putting the nested error in the beginning of the parent error.

@nesmabadr
Copy link
Contributor

We could also write a step using bash in the linter GitHub Action which fetches any values that don't match the pattern with err at the end, something like grep -nr 'fmt.Errorf("' . | grep -v '%w"' and it should fail if there are any.

medmes added a commit that referenced this issue Nov 29, 2024
* Resolve conflict

* Added error wrapping changes ##105

* Fix unit testing and wrapped errors #105

* Fix e2e testing

* Fix e2e testing

* remove assertion duplication as require.errorAs is included in the require.errorIs

* resolve error messages

* changes for the PR comment

* changes for the PR comment

* patch the path error message

* patch the path error message

* e2e cleanup err msg

* reformat the error message

* reformat the error message

* reformat the error message

* reformat the error message
@lindnerby lindnerby assigned medmes and unassigned nesmabadr Dec 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants