Revert "Revert "Ban functions that exit the process (#5359)" (#5365)" #5403
Conversation
| err := cmd.Execute() | ||
| if err != nil { | ||
| os.Exit(1) //nolint:forbidigo // this is OK inside the main function. | ||
| } |
There was a problem hiding this comment.
This was the bug in the previous version that broke the build. It was if err == nil { os.Exit(1) }. Oops
|
For the detailed report, please go to
|
@youngbupark what's the right thing to do when coverage goes down by 0.1% because of cosmetic changes? All my changes are in |
| return nil, err | ||
| } | ||
|
|
||
| configFile, err := GetConfigFilePath(config) |
There was a problem hiding this comment.
This added code degraded the coverage.
| @@ -137,15 +137,17 @@ func GetWorkspace(v *viper.Viper, name string) (*workspaces.Workspace, error) { | |||
| return section.GetWorkspace(name) | |||
There was a problem hiding this comment.
If you can add the test to validate the red block, you can pass the coverage rate.
for the detailed coverage report, you can go to Checks -> click Build and test -> download unit_test_coverage -> open html file.
There was a problem hiding this comment.
Why do I have to start writing tests unrelated to my change? This isn't something we should ask contributors to go through.
There was a problem hiding this comment.
Why do I have to start writing tests unrelated to my change? This isn't something we should ask contributors to go through.
You're right. The right approach is that you need to add the unit-test for what you made below in config.go. I found that zero test for config. the test for config file would be trickier due to the file access so I gave you some workaround :) I agree that we should guide them to write the test for their change.
| @@ -137,15 +137,17 @@ func GetWorkspace(v *viper.Viper, name string) (*workspaces.Workspace, error) { | |||
| return section.GetWorkspace(name) | |||
There was a problem hiding this comment.
Why do I have to start writing tests unrelated to my change? This isn't something we should ask contributors to go through.
You're right. The right approach is that you need to add the unit-test for what you made below in config.go. I found that zero test for config. the test for config file would be trickier due to the file access so I gave you some workaround :) I agree that we should guide them to write the test for their change.
| config := getConfig(configFilePath) | ||
| configFile := GetConfigFilePath(config) | ||
| config, err := getConfig(configFilePath) | ||
| if err != nil { |
rynowak
force-pushed
the
rynowak/linters
branch
from
9e3f3df to
f11ba98
Compare
|
For the detailed report, please go to
|
This reverts commit 05f8c80.
Description
This change reverts the revert of previous PR #5359. The previous iteration broke the build and was reverted.
Issue reference
Checklist
Please make sure you've completed the relevant tasks for this PR, out of the following list: