Skip to content
This repository has been archived by the owner on May 31, 2020. It is now read-only.

Add set.union and set.intersection aliases to set.__ior__ and set.__i… #684

Closed
wants to merge 2 commits into from
Closed

Conversation

lschumm
Copy link
Contributor

@lschumm lschumm commented Nov 12, 2017

set.union and set.intersection are two frequently used methods of a set. Issue #478 added the set.__ior__ and set.__iand__ methods which are equivalent–although .union and .intersection are more standard (as they aren't dunder methods) and are used in a few modules in the python standard library.

PR Checklist:

  • All new features have been tested
  • All new features have been documented
  • I have read the CONTRIBUTING.md file
  • I will abide by the code of conduct

Copy link
Member

@freakboy3742 freakboy3742 left a comment

Choose a reason for hiding this comment

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

The implementation looks right - but the test isn't actually testing what you think it is. Beef up the test, and this should be ready to go!

self.assertCodeExecution("""
a = {'a', 'b', 'c'}
b = {'a', 'c', 'd'}
a.union(b)
Copy link
Member

Choose a reason for hiding this comment

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

Although this is executes the right code, it won't actually test anything. assertCodeExecution() performs a comparison of the output of executed code - so, this test will verify that the code doesn't output anything. This will verify that the code runs without error, but won't verify that the output is actually correct.

To test that the output is correct, you have to print the result of the operation so that there is something to compare.

self.assertCodeExecution("""
a = {'a', 'b', 'c'}
b = {'a', 'c', 'd'}
a.intersection(b)
Copy link
Member

Choose a reason for hiding this comment

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

As with union, a print statement (or something else that generates output) is required.

self.assertCodeExecution("""
a = {'a', 'b', 'c'}
b = {'a', 'c', 'd'}
a.__ior__(b)
Copy link
Member

Choose a reason for hiding this comment

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

The check of __ior__ isn't needed - it's covered automatically by the InlineBinaryOperationTests.

self.assertCodeExecution("""
a = {'a', 'b', 'c'}
b = {'a', 'c', 'd'}
a.__iand__(b)
Copy link
Member

Choose a reason for hiding this comment

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

The check of __iand__ isn't needed - it's covered automatically by the InlineBinaryOperationTests.

@phildini
Copy link
Member

Hi there! It looks like this PR might be dead, so we're closing it for now. Feel free to re-open it if you'd like to continue, or think about directing your efforts to https://github.com/beeware/briefcase or https://github.com/beeware/toga. Both of these have more active development right now. 😄

@phildini phildini closed this Apr 25, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants