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

simplifying if-else chains to switches #6208

Merged
merged 10 commits into from Aug 6, 2018

Conversation

Projects
None yet
5 participants
@cristaloleg
Copy link
Contributor

cristaloleg commented Jul 29, 2018

Description

Simplifying if-else chains to switches.
All the changes were suggested by https://github.com/go-critic/go-critic

Motivation and Context

To make code easier to read.

How Has This Been Tested?

Regular build, there is no logical changes, only style and code simplification.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added unit tests to cover my changes.
  • I have added/updated functional tests in mint. (If yes, add mint PR # here: )
  • All new and existing tests passed.
@codecov

This comment has been minimized.

Copy link

codecov bot commented Jul 29, 2018

Codecov Report

Merging #6208 into master will increase coverage by 0.05%.
The diff coverage is 55.85%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6208      +/-   ##
==========================================
+ Coverage   58.41%   58.46%   +0.05%     
==========================================
  Files         222      222              
  Lines       32058    32043      -15     
==========================================
+ Hits        18726    18734       +8     
+ Misses      11689    11685       -4     
+ Partials     1643     1624      -19
Impacted Files Coverage Δ
cmd/common-main.go 13.51% <0%> (-0.08%) ⬇️
cmd/httprange.go 91.54% <100%> (ø) ⬆️
cmd/bool-flag.go 100% <100%> (ø) ⬆️
cmd/admin-rpc-client.go 51.43% <100%> (ø) ⬆️
cmd/admin-handlers.go 55.05% <100%> (+0.1%) ⬆️
cmd/xl-sets.go 55.95% <100%> (+0.86%) ⬆️
cmd/object-api-errors.go 57.63% <100%> (+0.2%) ⬆️
cmd/fs-v1-helpers.go 69.93% <25%> (+0.09%) ⬆️
cmd/postpolicyform.go 70.62% <33.33%> (-0.7%) ⬇️
cmd/posix.go 70.06% <42.3%> (+0.27%) ⬆️
... and 9 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update eabfcea...7dd973e. Read the comment docs.

@@ -230,7 +230,7 @@ func (c cacheObjects) GetObject(ctx context.Context, bucket, object string, star
return err
}
go func() {
if err = GetObjectFn(ctx, bucket, object, 0, objInfo.Size, io.MultiWriter(writer, pipeWriter), etag); err != nil {

This comment has been minimized.

Copy link
@harshavardhana

harshavardhana Jul 29, 2018

Member

These changes are not related to the changes mentioned by this PR, would it be possible to send this separately?

This comment has been minimized.

Copy link
@cristaloleg

cristaloleg Jul 29, 2018

Author Contributor

Yep, sure.

@@ -330,12 +330,12 @@ func CreateEndpoints(serverAddr string, args ...[]string) (string, EndpointList,
host = endpoint.Host
}
hostIPSet, _ := getHostIP4(host)
if IPSet, ok := pathIPMap[endpoint.Path]; ok {
if !IPSet.Intersection(hostIPSet).IsEmpty() {
if ipSet, ok := pathIPMap[endpoint.Path]; ok {

This comment has been minimized.

Copy link
@harshavardhana

harshavardhana Jul 29, 2018

Member

Keep cleanups to separate PR, this doesn't fix anything related to if-else or cyclomatic complexity.

This comment has been minimized.

Copy link
@cristaloleg

cristaloleg Jul 29, 2018

Author Contributor

ok

@@ -20,6 +20,6 @@ package cmd

// Fallocate is not POSIX and not supported under Windows
// Always return successful
func Fallocate(fd int, offset int64, len int64) error {
func Fallocate(fd int, offset int64, length int64) error {

This comment has been minimized.

Copy link
@harshavardhana

harshavardhana Jul 29, 2018

Member

Same here len is synonymous to length, no reason to rename it. The corresponding function in fallocate_linux.go is untouched..

This comment has been minimized.

Copy link
@cristaloleg

cristaloleg Jul 29, 2018

Author Contributor

It's a builtin function https://golang.org/pkg/builtin/#len

@@ -283,9 +284,9 @@ func LogIf(ctx context.Context, err error) {
req = &ReqInfo{API: "SYSTEM"}
}

API := "SYSTEM"
apiName := "SYSTEM"

This comment has been minimized.

Copy link
@harshavardhana

harshavardhana Jul 29, 2018

Member

Same here avoid cleanups like this....

This comment has been minimized.

Copy link
@cristaloleg

cristaloleg Jul 29, 2018

Author Contributor

ok

@@ -25,8 +25,7 @@ import (
// Function not implemented error
func isSysErrNoSys(err error) bool {
if pathErr, ok := err.(*os.PathError); ok {
switch pathErr.Err {
case syscall.ENOSYS:
if pathErr.Err == syscall.ENOSYS {

This comment has been minimized.

Copy link
@harshavardhana

harshavardhana Jul 29, 2018

Member

Further simplification is possible simply do return pathErr.Err == syscall.ENOSYS

This comment has been minimized.

Copy link
@cristaloleg

cristaloleg Jul 29, 2018

Author Contributor

indeed

@@ -58,8 +55,7 @@ func isSysErrNoSpace(err error) bool {
// Input/output error
func isSysErrIO(err error) bool {
if pathErr, ok := err.(*os.PathError); ok {
switch pathErr.Err {
case syscall.EIO:
if pathErr.Err == syscall.EIO {

This comment has been minimized.

Copy link
@harshavardhana

harshavardhana Jul 29, 2018

Member

Further simplification is possible simply do return pathErr.Err == syscall.EIO

@@ -69,8 +65,7 @@ func isSysErrIO(err error) bool {
// Check if the given error corresponds to EISDIR (is a directory).
func isSysErrIsDir(err error) bool {
if pathErr, ok := err.(*os.PathError); ok {
switch pathErr.Err {
case syscall.EISDIR:
if pathErr.Err == syscall.EISDIR {

This comment has been minimized.

Copy link
@harshavardhana

harshavardhana Jul 29, 2018

Member

Further simplification is possible simply do return pathErr.Err == syscall.EISDIR

@@ -80,8 +75,7 @@ func isSysErrIsDir(err error) bool {
// Check if the given error corresponds to ENOTDIR (is not a directory).
func isSysErrNotDir(err error) bool {
if pathErr, ok := err.(*os.PathError); ok {
switch pathErr.Err {
case syscall.ENOTDIR:
if pathErr.Err == syscall.ENOTDIR {

This comment has been minimized.

Copy link
@harshavardhana

harshavardhana Jul 29, 2018

Member

Further simplification is possible simply do return pathErr.Err == syscall.ENOTDIR

@@ -91,8 +85,7 @@ func isSysErrNotDir(err error) bool {
// Check if the given error corresponds to the ENAMETOOLONG (name too long).
func isSysErrTooLong(err error) bool {
if pathErr, ok := err.(*os.PathError); ok {
switch pathErr.Err {
case syscall.ENAMETOOLONG:
if pathErr.Err == syscall.ENAMETOOLONG {

This comment has been minimized.

Copy link
@harshavardhana

harshavardhana Jul 29, 2018

Member

Further simplification is possible simply do return pathErr.Err == syscall.ENAMETOOLONG

@@ -61,7 +61,7 @@ var globalRandomSource = rand.New(&lockedRandSource{
// until the maximum retry attempts are reached. - this function is a fully
// configurable version, meant for only advanced use cases. For the most part
// one should use newRetryTimerSimple and newRetryTimer.
func newRetryTimerWithJitter(unit time.Duration, cap time.Duration, jitter float64, doneCh chan struct{}) <-chan int {
func newRetryTimerWithJitter(unit time.Duration, max time.Duration, jitter float64, doneCh chan struct{}) <-chan int {

This comment has been minimized.

Copy link
@harshavardhana

harshavardhana Jul 29, 2018

Member

Do not change the variable name cap - any good reason you change it?

This comment has been minimized.

Copy link
@cristaloleg
@cristaloleg
Copy link
Contributor Author

cristaloleg left a comment

Thanks for the quick review. Will address suggestions quickly.

@@ -230,7 +230,7 @@ func (c cacheObjects) GetObject(ctx context.Context, bucket, object string, star
return err
}
go func() {
if err = GetObjectFn(ctx, bucket, object, 0, objInfo.Size, io.MultiWriter(writer, pipeWriter), etag); err != nil {

This comment has been minimized.

Copy link
@cristaloleg

cristaloleg Jul 29, 2018

Author Contributor

Yep, sure.

@@ -330,12 +330,12 @@ func CreateEndpoints(serverAddr string, args ...[]string) (string, EndpointList,
host = endpoint.Host
}
hostIPSet, _ := getHostIP4(host)
if IPSet, ok := pathIPMap[endpoint.Path]; ok {
if !IPSet.Intersection(hostIPSet).IsEmpty() {
if ipSet, ok := pathIPMap[endpoint.Path]; ok {

This comment has been minimized.

Copy link
@cristaloleg

cristaloleg Jul 29, 2018

Author Contributor

ok

@@ -283,9 +284,9 @@ func LogIf(ctx context.Context, err error) {
req = &ReqInfo{API: "SYSTEM"}
}

API := "SYSTEM"
apiName := "SYSTEM"

This comment has been minimized.

Copy link
@cristaloleg

cristaloleg Jul 29, 2018

Author Contributor

ok

@@ -25,8 +25,7 @@ import (
// Function not implemented error
func isSysErrNoSys(err error) bool {
if pathErr, ok := err.(*os.PathError); ok {
switch pathErr.Err {
case syscall.ENOSYS:
if pathErr.Err == syscall.ENOSYS {

This comment has been minimized.

Copy link
@cristaloleg

cristaloleg Jul 29, 2018

Author Contributor

indeed

@@ -20,6 +20,6 @@ package cmd

// Fallocate is not POSIX and not supported under Windows
// Always return successful
func Fallocate(fd int, offset int64, len int64) error {
func Fallocate(fd int, offset int64, length int64) error {

This comment has been minimized.

Copy link
@cristaloleg

cristaloleg Jul 29, 2018

Author Contributor

It's a builtin function https://golang.org/pkg/builtin/#len

@@ -61,7 +61,7 @@ var globalRandomSource = rand.New(&lockedRandSource{
// until the maximum retry attempts are reached. - this function is a fully
// configurable version, meant for only advanced use cases. For the most part
// one should use newRetryTimerSimple and newRetryTimer.
func newRetryTimerWithJitter(unit time.Duration, cap time.Duration, jitter float64, doneCh chan struct{}) <-chan int {
func newRetryTimerWithJitter(unit time.Duration, max time.Duration, jitter float64, doneCh chan struct{}) <-chan int {

This comment has been minimized.

Copy link
@cristaloleg

@cristaloleg cristaloleg force-pushed the cristaloleg:master branch from aae6b6e to 1c53552 Jul 29, 2018

@kannappanr kannappanr requested review from harshavardhana, ebozduman and kannappanr and removed request for harshavardhana Jul 30, 2018

@@ -56,11 +56,12 @@ func (bf *BoolFlag) UnmarshalJSON(data []byte) (err error) {

// ParseBoolFlag - parses string into BoolFlag.
func ParseBoolFlag(s string) (bf BoolFlag, err error) {
if s == "on" {
switch {

This comment has been minimized.

Copy link
@kannappanr

kannappanr Jul 31, 2018

Collaborator

this can be changed to

switch s {
	case "on":
		bf = true
	case "off":
		bf = false
	default:
                err = fmt.Errorf("invalid value ‘%s’ for BoolFlag", s)
	}

This comment has been minimized.

Copy link
@cristaloleg

cristaloleg Jul 31, 2018

Author Contributor

done

@kannappanr
Copy link
Collaborator

kannappanr left a comment

LGTM

return fmt.Errorf("%s (%s)->(%s)", errCrossDeviceLink, srcFilePath, dstFilePath)
} else if os.IsNotExist(err) {
case os.IsNotExist(err):
return errFileNotFound
}

This comment has been minimized.

Copy link
@ebozduman

ebozduman Jul 31, 2018

Collaborator

Don't we need default here (although it was even also missing in the original code);

default:
    return err

@cristaloleg cristaloleg dismissed stale reviews from kannappanr and harshavardhana via fea64af Jul 31, 2018

@@ -95,6 +95,8 @@ func renameAll(srcFilePath, dstFilePath string) (err error) {
return fmt.Errorf("%s (%s)->(%s)", errCrossDeviceLink, srcFilePath, dstFilePath)
case os.IsNotExist(err):
return errFileNotFound
default:
return err
}
}
return err

This comment has been minimized.

Copy link
@kannappanr

kannappanr Jul 31, 2018

Collaborator

It will look cleaner, if it is changed to return nil

This comment has been minimized.

Copy link
@ebozduman

ebozduman Aug 3, 2018

Collaborator

@cristaloleg,
Could you please incorporate @kannappanr's comment?
Thanks

@kannappanr kannappanr referenced this pull request Aug 2, 2018

Merged

Add reliable RemoveAll to handle racy situations #6227

3 of 9 tasks complete
@harshavardhana

This comment has been minimized.

Copy link
Member

harshavardhana commented Aug 3, 2018

Can you fix the conflicts @cristaloleg ?

@cristaloleg cristaloleg force-pushed the cristaloleg:master branch from 9b6aa09 to 63b3124 Aug 4, 2018

@@ -83,17 +83,20 @@ func renameAll(srcFilePath, dstFilePath string) (err error) {
}

if err = reliableRename(srcFilePath, dstFilePath); err != nil {
if isSysErrNotDir(err) {
switch {

This comment has been minimized.

Copy link
@kannappanr

kannappanr Aug 4, 2018

Collaborator

Sorry, I think you misunderstood what I meant. I wanted the renameAll function to look like this

func renameAll(srcFilePath, dstFilePath string) (err error) {
	if srcFilePath == "" || dstFilePath == "" {
		return errInvalidArgument
	}

	if err = checkPathLength(srcFilePath); err != nil {
		return err
	}
	if err = checkPathLength(dstFilePath); err != nil {
		return err
	}

	if err = reliableRename(srcFilePath, dstFilePath); err != nil {
		switch {
		case isSysErrNotDir(err):
			return errFileAccessDenied
		case isSysErrPathNotFound(err):
			// This is a special case should be handled only for
			// windows, because windows API does not return "not a
			// directory" error message. Handle this specifically here.
			return errFileAccessDenied
		case isSysErrCrossDevice(err):
			return fmt.Errorf("%s (%s)->(%s)", errCrossDeviceLink, srcFilePath, dstFilePath)
		case os.IsNotExist(err):
			return errFileNotFound
		default:
			return err
		}
	}
	return nil
}

This comment has been minimized.

Copy link
@cristaloleg

cristaloleg Aug 4, 2018

Author Contributor

Oh, now it's clear 😉

@kannappanr
Copy link
Collaborator

kannappanr left a comment

LGTM.

@minio-ops

This comment has been minimized.

Copy link

minio-ops commented Aug 4, 2018

Mint Automation

Test Result
mint-gateway-s3.sh ✔️
mint-gateway-azure.sh ✔️
mint-xl.sh ✔️
mint-gateway-nas.sh ✔️
mint-large-bucket.sh ✔️
mint-worm.sh ✔️
mint-fs.sh ✔️
mint-dist-xl.sh more...

6208-7dd973e/mint-dist-xl.sh.log:

Running with
SERVER_ENDPOINT:      minikube:31146
ACCESS_KEY:           minio
SECRET_KEY:           ***REDACTED***
ENABLE_HTTPS:         0
SERVER_REGION:        us-east-1
MINT_DATA_DIR:        /mint/data
MINT_MODE:            full
ENABLE_VIRTUAL_STYLE: 0

To get logs, run 'docker cp 200e1f766a5c:/mint/log /tmp/mint-logs'
(1/13) Running awscli tests ... done in 52 seconds
(2/13) Running aws-sdk-go tests ... done in 0 seconds
(3/13) Running aws-sdk-java tests ... done in 1 seconds
(4/13) Running aws-sdk-php tests ... done in 42 seconds
(5/13) Running aws-sdk-ruby tests ... done in 7 seconds
(6/13) Running mc tests ... done in 20 seconds
(7/13) Running minio-dotnet tests ... FAILED in 2 minutes and 34 seconds

Unhandled Exception: System.AggregateException: One or more errors occurred. (One or more errors occurred. (Minio API responded with message=Multiple disks failures, unable to write data.) (Minio API responded with message=Multiple disks failures, unable to write data.) (Minio API responded with message=Multiple disks failures, unable to write data.)) ---> System.AggregateException: One or more errors occurred. (Minio API responded with message=Multiple disks failures, unable to write data.) (Minio API responded with message=Multiple disks failures, unable to write data.) (Minio API responded with message=Multiple disks failures, unable to write data.) ---> Minio.Exceptions.MinioException: Minio API responded with message=Multiple disks failures, unable to write data.
   at Minio.MinioClient.ParseError(IRestResponse response)
   at Minio.MinioClient.<>c.<.ctor>b__76_0(IRestResponse response)
   at Minio.MinioClient.HandleIfErrorResponse(IRestResponse response, IEnumerable`1 handlers, DateTime startTime)
   at Minio.MinioClient.<ExecuteTaskAsync>d__79.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at System.Runtime.CompilerServices.ConfiguredTaskAwaitable`1.ConfiguredTaskAwaiter.GetResult()
   at Minio.MinioClient.<PutObjectAsync>d__20.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at System.Runtime.CompilerServices.ConfiguredTaskAwaitable`1.ConfiguredTaskAwaiter.GetResult()
   at Minio.MinioClient.<PutObjectAsync>d__15.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at System.Runtime.CompilerServices.TaskAwaiter.GetResult()
   at Minio.Functional.Tests.FunctionalTest.<PutObject_Task>d__52.MoveNext() in /mint/run/core/minio-dotnet/FunctionalTest.cs:line 741
   --- End of inner exception stack trace ---
   at System.Threading.Tasks.Task.ThrowIfExceptional(Boolean includeTaskCanceledExceptions)
   at System.Threading.Tasks.Task.Wait(Int32 millisecondsTimeout, CancellationToken cancellationToken)
   at System.Threading.Tasks.Task.Wait()
   at Minio.Functional.Tests.FunctionalTest.<RemoveObjects_Test2>d__75.MoveNext() in /mint/run/core/minio-dotnet/FunctionalTest.cs:line 1669
   --- End of inner exception stack trace ---
   at System.Threading.Tasks.Task.ThrowIfExceptional(Boolean includeTaskCanceledExceptions)
   at System.Threading.Tasks.Task.Wait(Int32 millisecondsTimeout, CancellationToken cancellationToken)
   at System.Threading.Tasks.Task.Wait()
   at Minio.Functional.Tests.FunctionalTest.Main(String[] args) in /mint/run/core/minio-dotnet/FunctionalTest.cs:line 220

Executed 6 out of 13 tests successfully.
@ebozduman
Copy link
Collaborator

ebozduman left a comment

LGTM

@kannappanr kannappanr merged commit 37de2db into minio:master Aug 6, 2018

3 of 4 checks passed

codecov/patch 55.85% of diff hit (target 58.41%)
Details
codecov/project 58.46% (+0.05%) compared to eabfcea
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@cristaloleg

This comment has been minimized.

Copy link
Contributor Author

cristaloleg commented Aug 6, 2018

Thanks to everyone 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.