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.

Sign up for GitHub

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

Jump to bottom

filestore: add "--file-order" option to "filestore ls" and "verify" #3938

Merged
merged 1 commit into from
May 24, 2017
Merged

filestore: add "--file-order" option to "filestore ls" and "verify" #3938

merged 1 commit into from
May 24, 2017

Conversation

kevina
Copy link
Contributor

@kevina kevina commented May 23, 2017

Closes #3922

@kevina kevina added the status/in-progress In progress label May 23, 2017
@kevina kevina mentioned this pull request May 23, 2017
Closed
@dokterbob
Copy link
Contributor

Just tested it and it's uncomparably faster! Thanks!

whyrusleeping
whyrusleeping reviewed May 23, 2017
View reviewed changes
filestore/util.go Outdated
return listAll(fs, false)
func ListAll(fs *Filestore, fileOrder bool) (func() *ListRes, error) {
if fileOrder {
println("ListAll With Sort")
Copy link
Member

Choose a reason for hiding this comment

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

don't use println

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sorry, that was meant for debugging and I forgot to remove it

whyrusleeping
whyrusleeping reviewed May 23, 2017
View reviewed changes
filestore/util.go Outdated
return nil
}
v := entries[i]
i += 1
Copy link
Member

Choose a reason for hiding this comment

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

i++

whyrusleeping
whyrusleeping approved these changes May 23, 2017
View reviewed changes
Copy link
Member

@whyrusleeping whyrusleeping left a comment

Choose a reason for hiding this comment

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

A couple comments, otherwise LGTM

@kevina kevina force-pushed the kevina/filestore-verify-sort branch from 3fd4cd5 to d548e32 Compare May 23, 2017 20:20
@kevina
Copy link
Contributor Author

kevina commented May 23, 2017

@whyrusleeping

A couple comments

Fixed.

@whyrusleeping
Copy link
Member

@kevina also a couple codeclimate issues

@kevina
Copy link
Contributor Author

kevina commented May 23, 2017

@whyrusleeping I know. It is being two picky. The check is "Indent Error Flow", but this is not an error flow. There is nothing is the style guide at https://github.com/golang/go/wiki/CodeReviewComments about this, the style guide is specially about error handling.

@kevina
Copy link
Contributor Author

kevina commented May 23, 2017

Also golint had this disclaimer in the README:

The suggestions made by golint are exactly that: suggestions. Golint is not perfect, and has both false positives and false negatives. Do not treat its output as a gold standard. We will not be adding pragmas or other knobs to suppress specific warnings, so do not expect or require code to be completely "lint-free". In short, this tool is not, and will never be, trustworthy enough for its suggestions to be enforced automatically, for example as part of a build process. Golint makes suggestions for many of the mechanically checkable items listed in Effective Go and the CodeReviewComments wiki page.

@whyrusleeping
Copy link
Member

@kevina you have redundant else statements. It makes the code cleaner to remove them

@kevina
filestore: add "--file-order" option to "filestore ls" and "verify"
332a796 
License: MIT
Signed-off-by: Kevin Atkinson <k@kevina.org>
@kevina kevina force-pushed the kevina/filestore-verify-sort branch from d548e32 to 332a796 Compare May 23, 2017 21:40
@kevina
Copy link
Contributor Author

kevina commented May 23, 2017

@whyrusleeping I don't agree, but its not worth an argument, fixed.

@kevina kevina force-pushed the kevina/filestore-verify-sort branch 2 times, most recently from d548e32 to 332a796 Compare May 24, 2017 04:03
@whyrusleeping whyrusleeping merged commit e61e4d2 into master May 24, 2017
@whyrusleeping whyrusleeping deleted the kevina/filestore-verify-sort branch May 24, 2017 15:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Kubuxu Kubuxu Kubuxu approved these changes

@whyrusleeping whyrusleeping whyrusleeping approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@kevina @dokterbob @whyrusleeping @Kubuxu