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

Add Map() and FilterFunc() #21

Closed
wants to merge 10 commits into from
Closed

Add Map() and FilterFunc() #21

wants to merge 10 commits into from

Conversation

AnirudhVyas
Copy link

Please review again and merge.

Anirudh Vyas added 6 commits March 17, 2017 17:43
…form properties to a map - this is useful when library makers do not want to expose any internal libraries they may be leveraging or force customers to depend on their data structures, map with filters provides more flexibility for users to filter an property in between
Copy link
Owner

@magiconair magiconair left a comment

Choose a reason for hiding this comment

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

See inline comments

properties.go Outdated
@@ -647,6 +622,33 @@ func (p *Properties) WriteComment(w io.Writer, prefix string, enc Encoding) (n i
return
}

// ToMap returns a copy of the properties as a map - useful when users of library are themselves
Copy link
Owner

Choose a reason for hiding this comment

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

Pls remove the - useful ... part since you don't know what users will want to use this for.

properties.go Outdated
return m
}

/// FilterFunc returns a copy of the properties which includes the values
Copy link
Owner

Choose a reason for hiding this comment

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

Only two slashes for the comment.

}

func TestProperties_ToMapWith(t *testing.T) {
func TestFilterFunc(t *testing.T) {
Copy link
Owner

Choose a reason for hiding this comment

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

func TestFilterFunc(t *testing.T) {
    input := "key=value\nabc=def"
    p := mustParse(t, input)
    pp := p.FilterFunc(func(k, v string) bool {
        return k != "abc"
    })
    m := map[string]string{"key": "value"}
    assert.Equal(t, pp.ToMap(), m)
}

@magiconair magiconair reopened this Mar 18, 2017
@AnirudhVyas
Copy link
Author

Done

Copy link
Owner

@magiconair magiconair left a comment

Choose a reason for hiding this comment

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

Last minor changes. I'm still struggling with the ToMap name a bit. Let me sleep on it and if I don't come up with something better by tomorrow then I'll merge it.

properties.go Outdated
@@ -622,6 +622,30 @@ func (p *Properties) WriteComment(w io.Writer, prefix string, enc Encoding) (n i
return
}

// ToMap returns a copy of the properties as a map
Copy link
Owner

Choose a reason for hiding this comment

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

Pls add a full stop at the end.

Copy link
Author

Choose a reason for hiding this comment

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

name it as Map() -- it is mirror image of String( ) ?

Copy link
Author

Choose a reason for hiding this comment

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

added full stop and pushed ...

Copy link
Author

Choose a reason for hiding this comment

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

Let me know -- I can rename and push - thanks.

Copy link
Owner

Choose a reason for hiding this comment

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

Maybe you can do a quick dig through the stdlib to find some precedence. I don't think they have ToXXX functions there. Map could work but might be too generic.

@@ -825,6 +825,22 @@ func TestMerge(t *testing.T) {
assert.Equal(t, p1.MustGet("key"), "another value")
assert.Equal(t, p1.GetComment("key"), "another comment")
}
func TestToMap(t *testing.T) {
Copy link
Owner

Choose a reason for hiding this comment

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

Pls add a blank line before.

@AnirudhVyas
Copy link
Author

blank line - added.

@AnirudhVyas
Copy link
Author

Please let me know if there's anything else that is needed; You may choose a better name instead of ToMap( ). Thanks for your help!

@magiconair
Copy link
Owner

The code looks good. Kids kept me busy today so I didn't get time to think about the name. Did you do any research in the stdlib by any chance? I should have time to merge this tomorrow.

@AnirudhVyas
Copy link
Author

Seems like you have handsful - My little one is in love with slides off late; so that kept me busy as well. I'll take a look tomorrow - Thanks.

@magiconair
Copy link
Owner

There are examples for both ToXXX() and Type() in the stdlib. With the current set of methods I think Map() is a better fit. I'll make the change and merge it.

magiconair pushed a commit that referenced this pull request Mar 20, 2017
* Map() exports the properties as a map[string]string.
* FilterFunc() allows filtering through a boolean function.
@magiconair magiconair changed the title Request for adding ToMap and FilterFunc (suggested from pull request #20) Add Map() and FilterFunc() Mar 20, 2017
@magiconair
Copy link
Owner

I've merged the PR but apparently not in a way that auto-closes this one. :( Attribution is on you, though.

Thanks for the change and your quick replies.

@magiconair magiconair closed this Mar 20, 2017
@magiconair magiconair added this to the 1.7.2 milestone Oct 31, 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