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

Feature/add hashing #30

Closed
wants to merge 13 commits into from
Closed

Feature/add hashing #30

wants to merge 13 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Jun 11, 2017

Getting this out because I'd like to hear your feedback.

Details of implementation dicussion can be found at #9

Copy link
Owner

@kashav kashav left a comment

Choose a reason for hiding this comment

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

Looks good overall, left some comments for things that may need changing.

Haven't tested yet, will update as I do that.

@@ -24,3 +24,11 @@ func upper(name string) interface{} {
func lower(name string) interface{} {
return strings.ToLower(name)
}

func truncate(str string, n int) string {
if len(str) < n || n > len(str) {
Copy link
Owner

Choose a reason for hiding this comment

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

Checking the same thing twice! 😄

Copy link
Author

Choose a reason for hiding this comment

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

lol 🤦‍♂️


func hash(info os.FileInfo, path string, hasher crypto.SignerOpts) (string, error) {
if info.IsDir() {
return "----------------------------------------", nil
Copy link
Owner

@kashav kashav Jun 11, 2017

Choose a reason for hiding this comment

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

Should use strings.Repeat with hasher.HashFunc().Size here, since sizes may vary between hash functions.

var hash string
hash, err = p.hash(crypto.SHA1)
if err == nil && len(p.Args) > 0 && p.Args[0] != "" {
if n, err := strconv.Atoi(p.Args[0]); err == nil {
Copy link
Owner

Choose a reason for hiding this comment

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

We don't want to reinitialize err here.

@@ -109,6 +122,30 @@ func (p *FormatParams) shortPath() (interface{}, error) {
return p.Info.Name(), nil
}

func (p *FormatParams) hash(hasher crypto.SignerOpts) (string, error) {
Copy link
Owner

Choose a reason for hiding this comment

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

Let's return an interface{} here (instead of string).

return hash(p.Info, p.Path, hasher)
}

func hash(info os.FileInfo, path string, hasher crypto.SignerOpts) (string, error) {
Copy link
Owner

Choose a reason for hiding this comment

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

interface{} here as well.


f, err := os.Open(path)
if err != nil {
return "", err
Copy link
Owner

Choose a reason for hiding this comment

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

This can now be changed to return nil (below as well).

case "hash":
v, err := hash(info, path, crypto.SHA1)
if err != nil {
panic(err.Error())
Copy link
Owner

Choose a reason for hiding this comment

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

Not sure how I feel about panicking, but you can leave this as is for now.

Might need to refactor this method to return an error.

Copy link
Author

Choose a reason for hiding this comment

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

That's what I was thinking as well, I suppose we can let the caller handle the error?

@kashav
Copy link
Owner

kashav commented Jun 11, 2017

Nice; seems to be working well!

Let me know when this is ready – I'm going to move this into another branch and make some changes (mostly test stuff) before merging with master.

@ghost
Copy link
Author

ghost commented Jun 11, 2017

I'm ok with this getting merged into a dev branch. I refactored it a bit more (extracting arguments and error handling with defaults), so you may want to review those some more.

@kashav kashav mentioned this pull request Jun 18, 2017
@kashav
Copy link
Owner

kashav commented Jun 20, 2017

Merged with #31. Thanks for the contribution, @jonesbrianc26!

@kashav kashav closed this Jun 20, 2017
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