Skip to content

bpo-35899: Support of empty and weird strings by Enum#11840

Closed
matrixise wants to merge 2 commits into
python:masterfrom
matrixise:bpo-35899
Closed

bpo-35899: Support of empty and weird strings by Enum#11840
matrixise wants to merge 2 commits into
python:masterfrom
matrixise:bpo-35899

Conversation

@matrixise

@matrixise matrixise commented Feb 13, 2019 •
edited by bedevere-bot
Loading

Copy link
Copy Markdown
Member

Co-authored-by: Maxwell maxwellpxt@gmail.com
Co-authored-by: Stéphane Wirtel stephane@wirtel.be

https://bugs.python.org/issue35899

tirkarthi
tirkarthi reviewed Feb 13, 2019
View reviewed changes
Comment thread Lib/test/test_enum.py Outdated

@tirkarthi tirkarthi Feb 13, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

There should be a test for the original report where empty string causes an error like Yay = Enum('Yay', ('', 'B', 'C')) in https://bugs.python.org/issue35899#msg334866

@matrixise matrixise Feb 14, 2019

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Hi @tirkarthi
Thank you for your feedback, I have just added this test.

bdbaraban
bdbaraban suggested changes Feb 14, 2019
View reviewed changes

@bdbaraban bdbaraban left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Building on @tirkarthi's comment, I'd like to offer alternative tests that would test both empty and "weird" strings while also aligning more with the existing unittest layout in test_enum.py. I propose the following unittest functions to be entered starting at line 762 in the original test_enum.py file:

def test_programmatic_function_string_list_with_empty_string(self):
     SummerMonth = Enum('SummerMonth', ['june', 'july', ''])
     lst = list(SummerMonth)
     self.assertEqual(len(lst), len(SummerMonth))
     self.assertEqual(len(SummerMonth), 3, SummerMonth)
     self.assertEqual(
             [SummerMonth.june, SummerMonth.july, getattr(SummerMonth, '')],
             lst
             )
    for i, month in enumerate(['june', 'july', ''], 1):
        e = SummerMonth(i)
        self.assertEqual(int(e.value), i)
        self.assertNotEqual(e, i)
        self.assertEqual(e.name, month)
        self.assertIn(e, SummerMonth)
        self.assertIs(type(e), SummerMonth)
 
def test_programmatic_function_string_list_with_weird_string(self):
    SummerMonth = Enum('SummerMonth', ['june', 'july', '!'])
    lst = list(SummerMonth)
    self.assertEqual(len(lst), len(SummerMonth))
    self.assertEqual(len(SummerMonth), 3, SummerMonth)
    self.assertEqual(
            [SummerMonth.june, SummerMonth.july, getattr(SummerMonth, '!')],
            lst
            )
    for i, month in enumerate('june july !'.split(), 1):
        e = SummerMonth(i)
        self.assertEqual(int(e.value), i)
        self.assertNotEqual(e, i)
        self.assertEqual(e.name, month)
        self.assertIn(e, SummerMonth)
        self.assertIs(type(e), SummerMonth)

@matrixise

matrixise commented Feb 14, 2019

Copy link
Copy Markdown
Member Author

Hi @bdbaraban

  1. You have to use the subTest() method when you want to create a test in a loop.
  2. Your test is really complete but the other tests (from test_enum.py) already cover all the features of Enum.
  3. A test should be the simplest as possible.

Thank you

matrixise and others added 2 commits February 14, 2019 08:39
@matrixise @Maxpxt
bpo-35899: Support of empty and weird strings by Enum
7a0480a 
Co-authored-by: Maxwell <maxwellpxt@gmail.com>
Co-authored-by: Stéphane Wirtel <stephane@wirtel.be>
@matrixise
Add the test with the empty string
b8b0554
@bdbaraban

bdbaraban commented Feb 14, 2019

Copy link
Copy Markdown
Contributor

Hi @bdbaraban

1. You have to use the subTest() method when you want to create a test in a loop.

2. Your test is really complete but the other tests (from test_enum.py) already cover all the features of Enum.

3. A test should be the simplest as possible.

Thank you

Understood, thank you for the feedback! For clarification purposes, I based these tests off the other programmatic_function tests defined in TestEnum. They also run multiple tests in a for loop, without using the subTest() method. Is this incorrect?

@matrixise

matrixise commented Feb 14, 2019

Copy link
Copy Markdown
Member Author

@bdbaraban
unittest.TestCase.subTest exists since 3.4, same thing for Enum.
the code of programmatic_function is not incorrect, but in the case of a failed test, you will start to debug the test and will try to find the variables/values with the bad domain. with a context (like subTest), you will have these variables when you will execute the tests.

For my part, I prefer to use subTest (or in the case of pytest, @pytest.mark.parametrize) because you will execute your test in a context and for the debugging, it's easiest. See https://docs.python.org/3/library/unittest.html?highlight=subtest#distinguishing-test-iterations-using-subtests

Now, when we write new code for Python, test or code, we don't use the new features of Python. For example, the PEP 572 has been merged into CPython, we won't start to rewrite the library and the tests with the new syntax. It's the same with the f-string.

@ethanfurman ethanfurman self-assigned this Feb 14, 2019
@matrixise

matrixise commented Feb 18, 2019

Copy link
Copy Markdown
Member Author

@ethanfurman I close this PR because you can use the PR of @bdbaraban.

Have a nice day,

Stéphane

@matrixise matrixise closed this Feb 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tirkarthi tirkarthi tirkarthi left review comments
+1 more reviewer
@bdbaraban bdbaraban bdbaraban requested changes
Reviewers whose approvals may not affect merge requirements

Assignees

@ethanfurman ethanfurman

Labels

awaiting core review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@matrixise @bdbaraban @tirkarthi @ethanfurman @the-knights-who-say-ni @bedevere-bot