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

Fixing issue #20 #22

Merged
merged 10 commits into from
Jul 15, 2017
Merged

Fixing issue #20 #22

merged 10 commits into from
Jul 15, 2017

Conversation

fclairamb
Copy link
Owner

We were adding spaces in the file listing for no reason.

We were adding spaces in the file listing for no reason.
I style doubt it will change antyhing (as the LIST command seems to be a bit broken).
This first implementation might have limitations but so far it works great.
Copy link
Collaborator

@mgenov mgenov left a comment

Choose a reason for hiding this comment

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

+2 for this change.

The CI is failing but tests are passing in the log. It looks like that there is an issue with the build.

break
}
fileName := line[47:]
//t.Logf("Line: \"%s\", File: \"%s\"", line, fileName)
Copy link
Collaborator

Choose a reason for hiding this comment

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

commented line with logging could be removed.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Yes it's the cyclomatic complexity of the test that has gone too high.

$ gocyclo -over 15 .
18 tests TestDirAccess tests/handle_dirs_test.go:8:1

There are lots of tests around code quality on the build stage to make sure we keep the code clean. We should also indeed prevent from having commented-out code.

I'm going to fix that.

@fclairamb fclairamb merged commit ddd3e0c into master Jul 15, 2017
@fclairamb fclairamb deleted the bugfix-issue-20-file-listing-with-space branch July 15, 2017 18:31
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.

2 participants