code review #132
Merged
code review #132
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
There is one more error in the code I would like to fix. It would require to refactor the code a bit, so I will wait to see if you are accepting this PR first 😄 |
|
You are not fun @HakanVardarr 😄 you also started fixing in #131 issue I have fixed there 😝 As you started fixing them before me, I will wait for your PR to be merged then I will fix the conflicts |
|
Sorry for that. :( |
|
Much appreciated @ccoVeille ❤️ Excited to take a look through the PR. |
|
For record, you can review but I will wait for the #131 to be merged. I'm confident in being able to fix the conflicts easily. |
|
@ccoVeille Once the merge conflicts are resolved I'll take a look at your PR next! |
|
Here we go ! |
files were formatted with gofumpt
This is the only way to be able to catch a specific error with errors.Is in the code calling the method
The existing code could lead to errors with wrapped errors - using `if err == whatever` don't catch wrapped errors - using type assertion could lead to panic
The code is then clearer
any is available since Go 1.18, it makes code clearer.
an option of cli doesn't need to be named cli.CLIOption
when importing something from cli package, we shouldn't name a variable cli
The code is clearer to understand.
|
I plan to add |
kehoecj
approved these changes
Apr 11, 2024
There was a problem hiding this comment.
@ccoVeille LGTM - thanks for the code review! I learned some new things 😄 Do you want me to merge this now or wait for the linter?
|
@kehoecj please merge I will rebase and push back the linter |
|
Also fixed #134 |
shiina4119
pushed a commit
to shiina4119/config-file-validator
that referenced
this pull request
Aug 23, 2024
* chore: format files before working on them
files were formatted with gofumpt
* fix: make sure to always wrap errors
This is the only way to be able to catch a specific error with errors.Is
in the code calling the method
* fix: errors have to be manipulated with care
The existing code could lead to errors with wrapped errors
- using `if err == whatever` don't catch wrapped errors
- using type assertion could lead to panic
* fix: error returned by reporters must be handled
* fix: misuage of WalkDirFunc
* chore: fix some typos and error in the struct
* chore: remove unused receivers
The code is then clearer
* chore: Replace all interface{} by any
any is available since Go 1.18, it makes code clearer.
* chore: use uppercase for JSON, XML, and YAML acronyms
* chore: use simpler name for cli Option
an option of cli doesn't need to be named cli.CLIOption
* chore: avoid shadowing import
when importing something from cli package, we shouldn't name a variable cli
* chore: avoid useless else
The code is clearer to understand.
* chore: remove unused assertErrorIs and accept assert.Error
shiina4119
pushed a commit
to shiina4119/config-file-validator
that referenced
this pull request
Oct 4, 2024
* chore: format files before working on them
files were formatted with gofumpt
* fix: make sure to always wrap errors
This is the only way to be able to catch a specific error with errors.Is
in the code calling the method
* fix: errors have to be manipulated with care
The existing code could lead to errors with wrapped errors
- using `if err == whatever` don't catch wrapped errors
- using type assertion could lead to panic
* fix: error returned by reporters must be handled
* fix: misuage of WalkDirFunc
* chore: fix some typos and error in the struct
* chore: remove unused receivers
The code is then clearer
* chore: Replace all interface{} by any
any is available since Go 1.18, it makes code clearer.
* chore: use uppercase for JSON, XML, and YAML acronyms
* chore: use simpler name for cli Option
an option of cli doesn't need to be named cli.CLIOption
* chore: avoid shadowing import
when importing something from cli package, we shouldn't name a variable cli
* chore: avoid useless else
The code is clearer to understand.
* chore: remove unused assertErrorIs and accept assert.Error
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I started looking at your code as I'm using it.
Some are existing errors, some are things that could become errors later with a refactoring
Here is a PR about fixing them.