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 intersect util method for StringSet #4

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

axamon
Copy link

@axamon axamon commented Apr 1, 2019

Intersect function for *StringSet types that is concurrent safe added.

@CLAassistant
Copy link

CLAassistant commented Apr 1, 2019

CLA assistant check
All committers have signed the CLA.

}

// Add testItems to testSet
for _, item := range testItems {
Copy link
Contributor

Choose a reason for hiding this comment

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

use string set Add method

Copy link
Author

Choose a reason for hiding this comment

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

I can do it but then tests will be different from previous ones.

}

// Intersect the two stringSet
intersection := testSet.Intersect(testSet1)
Copy link
Contributor

Choose a reason for hiding this comment

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

Use test table and add more unit tests such as

  • No intersection result
  • Empty sets
  • both set are purely different.

Copy link
Author

Choose a reason for hiding this comment

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

ok, done.

Copy link
Contributor

Choose a reason for hiding this comment

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

Use table driven tests, rather than creating separate tests.
https://github.com/golang/go/wiki/TableDrivenTests

Copy link
Author

Choose a reason for hiding this comment

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

cool stuff!
Thanks I understand what you meant now.
Tests unified in one according to TableDrivenTests.

@zhixinwen
Copy link
Contributor

I do not think we should add the func if there is no use case.

@varungup90
Copy link
Contributor

I do not think we should add the func if there is no use case.

That's fine.

@varungup90
Copy link
Contributor

@axamon Update the commit message to be more meaningful.

@axamon
Copy link
Author

axamon commented Apr 2, 2019

I added the intersect function for *StringSet types that is concurrency safe.

@varungup90
Copy link
Contributor

I added the intersect function for *StringSet types that is concurrency safe.

Can you change commit name from "added intersect" to something "Add intersect util method for StringSet"

As well as change commit message from "might be useful" -> relevant to the PR.

@axamon axamon changed the title added intersect Add intersect util method for StringSet Apr 3, 2019
@varungup90
Copy link
Contributor

@axamon Thank for the fixes, we do not have unit & integration test CI setup at open source yet. I will run the patch locally, if no errors then I will merge the PR.

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.

4 participants