Format enums with one value per line #216

altendky · 2017-10-12T18:20:07Z

Issue #213

altendky · 2017-10-12T18:23:22Z

I'm not particularly confident that the punctuation parameter is the proper approach. Perhaps I should call self.visit() directly on the assignment in _generate_enum_body()?

altendky · 2017-10-12T18:59:26Z

Incomplete tests, fixing them and fixing a bug with 'empty' enums such as used in typedefs.

eliben · 2017-10-13T03:07:38Z

tests/test_c_generator.py

@@ -280,6 +281,25 @@ def test_compound_literal(self):
        self._assert_ctoc_correct('int i = ++(int){ 1 };')
        self._assert_ctoc_correct('struct foo_s foo = (struct foo_s){ 1, 2 };')

+    def test_enum(self):
+        s = textwrap.dedent(r'''


Is this dedent necessary? none of the other tests use it since the code is being parsed anyway

_assert_ctoc_correct() is not checking the formatting and my change is a formatting change so it would seem that my test should do at least something different. I just verified this by checking that all three of the following tests pass.

def test_enum_a(self): s = textwrap.dedent(r''' enum e { a = 1, b = 2, c = 3 }; '''[1:]) self._assert_ctoc_correct(s) def test_enum_b(self): s = r''' enum e { a = 1, b = 2, c = 3 }; ''' self._assert_ctoc_correct(s) def test_enum_c(self): s = r'''enum e {a = 1, b = 2, c = 3};''' self._assert_ctoc_correct(s)

Ah, I see. If that's the case, no problem. The other tests don't tend to do one-to-one comparison of actual formatting, but I guess it's OK in this case. I'm a bit worried that this test will be fragile because it compares large strings, but we can fix it if it becomes and issue.

eliben · 2017-10-13T03:09:05Z

pycparser/c_generator.py

+        if name in ('struct', 'union'):
+            members = n.decls
+            body_function = self._generate_struct_union_body
+        elif name in ('enum',):


this should probably be:

else: assert name == 'enum' ... rest of code

It is my understanding that assert is only proper in test code. But, the gist that an exception should be raised for an invalid input does sound good. How about leaving the elif and adding an else that raises a to-be-defined exception?

UnknownTypeError('`{name}` not recognized'.format(name=name))

Given that this is an internal method it seems that we may not need a nice user error. I'll note that as is we'd get an UnboundLocalError for the following access to members. I added a test for that and will adjust it based on the implementation decision.

I actually think assert is fine in this case, since it's checking an invariant in the code - it's a bug if this place is reached, so assert is appropriate.

eliben · 2017-10-13T03:09:37Z

pycparser/c_generator.py

+            body_function = self._generate_struct_union_body
+        elif name in ('enum',):
+            if n.values is None:
+                members = ()


members = () if n.values is None else n.values.enumerators

Pending above possibilities I'll change this.

eliben · 2017-10-13T03:11:02Z

pycparser/c_generator.py

+            if i == len(members) - 1:
+                punctuation = ''
+
+            s.append(self._generate_stmt(


I wonder why you need to build an AST node again here, why won't just emitting the text do?

I was trying to make it look like how struct and union were done, but failing. I removed the punctuation parameter from _generate_stmt() and added visit_Enumerator(). Perhaps this will fit better with the existing code.

…_enum()

altendky

Thanks for the very prompt review.

I pushed a couple commits but let me know how you want to proceed on the remaining points.

altendky · 2017-10-13T15:03:40Z

tests/test_c_generator.py

@@ -280,6 +281,25 @@ def test_compound_literal(self):
        self._assert_ctoc_correct('int i = ++(int){ 1 };')
        self._assert_ctoc_correct('struct foo_s foo = (struct foo_s){ 1, 2 };')

+    def test_enum(self):
+        s = textwrap.dedent(r'''


_assert_ctoc_correct() is not checking the formatting and my change is a formatting change so it would seem that my test should do at least something different. I just verified this by checking that all three of the following tests pass.

def test_enum_a(self): s = textwrap.dedent(r''' enum e { a = 1, b = 2, c = 3 }; '''[1:]) self._assert_ctoc_correct(s) def test_enum_b(self): s = r''' enum e { a = 1, b = 2, c = 3 }; ''' self._assert_ctoc_correct(s) def test_enum_c(self): s = r'''enum e {a = 1, b = 2, c = 3};''' self._assert_ctoc_correct(s)

altendky · 2017-10-13T15:16:17Z

pycparser/c_generator.py

+        if name in ('struct', 'union'):
+            members = n.decls
+            body_function = self._generate_struct_union_body
+        elif name in ('enum',):


It is my understanding that assert is only proper in test code. But, the gist that an exception should be raised for an invalid input does sound good. How about leaving the elif and adding an else that raises a to-be-defined exception?

UnknownTypeError('`{name}` not recognized'.format(name=name))

Given that this is an internal method it seems that we may not need a nice user error. I'll note that as is we'd get an UnboundLocalError for the following access to members. I added a test for that and will adjust it based on the implementation decision.

altendky · 2017-10-13T15:16:19Z

pycparser/c_generator.py

+            body_function = self._generate_struct_union_body
+        elif name in ('enum',):
+            if n.values is None:
+                members = ()


Pending above possibilities I'll change this.

altendky · 2017-10-13T15:39:14Z

pycparser/c_generator.py

+            if i == len(members) - 1:
+                punctuation = ''
+
+            s.append(self._generate_stmt(


I was trying to make it look like how struct and union were done, but failing. I removed the punctuation parameter from _generate_stmt() and added visit_Enumerator(). Perhaps this will fit better with the existing code.

eliben · 2017-10-14T03:36:26Z

tests/test_c_generator.py

@@ -280,6 +281,25 @@ def test_compound_literal(self):
        self._assert_ctoc_correct('int i = ++(int){ 1 };')
        self._assert_ctoc_correct('struct foo_s foo = (struct foo_s){ 1, 2 };')

+    def test_enum(self):
+        s = textwrap.dedent(r'''


Ah, I see. If that's the case, no problem. The other tests don't tend to do one-to-one comparison of actual formatting, but I guess it's OK in this case. I'm a bit worried that this test will be fragile because it compares large strings, but we can fix it if it becomes and issue.

eliben · 2017-10-14T03:38:19Z

pycparser/c_generator.py

+
+    def _generate_enum_body(self, members):
+        s = []
+


Python usually prefers denser style - none of the empty lines in this function are really necessary

eliben · 2017-10-14T03:39:05Z

pycparser/c_generator.py

+    def _generate_enum_body(self, members):
+        s = []
+
+        for value in members:


Actually, this loop can be turned into a simple list comprehension, no?

Yes, I refactored and it got simpler and I missed that last step.

eliben · 2017-10-14T03:39:21Z

pycparser/c_generator.py

+
+            s.append(line)
+
+        return ''.join(s)[:-2] + '\n'


A comment should say why the [:-2] is needed here

Format enums with one value per line

fbf979f

Issue eliben#213

Fix empty enum generation

529275e

eliben requested changes Oct 13, 2017

View reviewed changes

altendky added 2 commits October 13, 2017 11:39

Refactor the generation of enumerator values

afdf8c2

Add test for invalid name passed to CGenerator._generate_struct_union…

8bde5f9

…_enum()

altendky commented Oct 13, 2017

View reviewed changes

Add back missing blank line

9cf8316

eliben requested changes Oct 14, 2017

View reviewed changes

altendky added 4 commits October 16, 2017 09:27

Make sure passed type name is valid in _generate_struct_union_enum()

d248b3b

Switch an if/else to a ternary

fe28227

Remove blank lines

4b6d283

Refactor _generate_enum_body() to a genexp

b19d2b7

eliben approved these changes Oct 19, 2017

View reviewed changes

eliben merged commit 7547e85 into eliben:master Oct 19, 2017

natezb mentioned this pull request Mar 8, 2018

Valueless enum members are generated incorrectly #240

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Format enums with one value per line #216

Format enums with one value per line #216

altendky commented Oct 12, 2017

altendky commented Oct 12, 2017

altendky commented Oct 12, 2017

eliben Oct 13, 2017

altendky Oct 13, 2017

eliben Oct 14, 2017

eliben Oct 13, 2017

altendky Oct 13, 2017

eliben Oct 14, 2017

eliben Oct 13, 2017

altendky Oct 13, 2017

eliben Oct 13, 2017

altendky Oct 13, 2017

altendky left a comment

altendky Oct 13, 2017

altendky Oct 13, 2017

altendky Oct 13, 2017

altendky Oct 13, 2017

eliben Oct 14, 2017

eliben Oct 14, 2017

eliben Oct 14, 2017

altendky Oct 16, 2017

eliben Oct 14, 2017

Format enums with one value per line #216

Format enums with one value per line #216

Conversation

altendky commented Oct 12, 2017

altendky commented Oct 12, 2017

altendky commented Oct 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

altendky left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment