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

Edits #78

Draft
wants to merge 7 commits into
base: master
Choose a base branch
from
Draft

Edits #78

wants to merge 7 commits into from

Conversation

ryandmaggio
Copy link

Made some changes to parser and ast to support case_clauses and patterns. Submitting as a draft PR because I'm not sure if it is correct (it looks correct to me on the example I tested on, but I don't know how to compare it to the "canonical" ast).

Copy link
Owner

@idank idank left a comment

Choose a reason for hiding this comment

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

I did a pass and it generally looks good to me. Granted I haven't touched the parser in years so we're learning together. :-)

Left a few small comments, let's fix those and add some tests? Thanks for the contribution!


def p_pattern_list(p):
'''pattern_list : newline_list pattern RIGHT_PAREN compound_list
| newline_list pattern RIGHT_PAREN newline_list
| newline_list LEFT_PAREN pattern RIGHT_PAREN compound_list
| newline_list LEFT_PAREN pattern RIGHT_PAREN newline_list'''
handleNotImplemented(p, 'pattern list')

parserobj = p.context
Copy link
Owner

Choose a reason for hiding this comment

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

seems unused

Copy link
Author

Choose a reason for hiding this comment

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

Taken care of

rparen = ast.node(kind='reservedword', word=p[4], pos=p.lexspan(4))
parts.extend([lparen, patterns, rparen])
if p.slice[-1].type == "compound_list":
# for some reason, p[-1] does not give the "true" last element, do not know why
Copy link
Owner

Choose a reason for hiding this comment

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

Copy link
Author

Choose a reason for hiding this comment

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

Thanks!

tests/test_parser.py Show resolved Hide resolved
@ryandmaggio
Copy link
Author

I've pushed my updated code which I believe to be correct (although resulting in a somewhat unintuitive AST, in my opinion, but it seems right). Locally, "make tests" doesn't work, it gives an error in the "malformed if" case, which is odd since I didn't touch that, to my knowledge.

Copy link
Owner

@idank idank left a comment

Choose a reason for hiding this comment

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

looks good overall!

  1. can you please look at the test failures
  2. please squash the commits, i think you want to end up with one?

redirects = [],
list=[ast.node(kind='case', parts=parts, pos=_partsspan(parts))],
pos=_partsspan(parts))
#handleNotImplemented(p, 'case command')
Copy link
Owner

Choose a reason for hiding this comment

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

left over

@@ -377,14 +382,36 @@ def p_elif_clause(p):
def p_case_clause(p):
'''case_clause : pattern_list
| case_clause_sequence pattern_list'''
handleNotImplemented(p, 'case clause')

Copy link
Owner

Choose a reason for hiding this comment

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

unnecessary new line here


def p_pattern_list(p):
'''pattern_list : newline_list pattern RIGHT_PAREN compound_list
| newline_list pattern RIGHT_PAREN newline_list
| newline_list LEFT_PAREN pattern RIGHT_PAREN compound_list
| newline_list LEFT_PAREN pattern RIGHT_PAREN newline_list'''
handleNotImplemented(p, 'pattern list')

Copy link
Owner

Choose a reason for hiding this comment

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

same

patterns = p[3]
rparen = ast.node(kind='reservedword', word=p[4], pos=p.lexspan(4))
parts.extend([lparen, patterns, rparen])
if p.slice[-1].type == "compound_list":
Copy link
Owner

Choose a reason for hiding this comment

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

please add a comment that explains this part


def p_pattern(p):
'''pattern : WORD
| pattern BAR WORD'''
handleNotImplemented(p, 'pattern')

Copy link
Owner

Choose a reason for hiding this comment

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

newline

;;
esac
"""
self.assertASTEquals(s,
Copy link
Owner

Choose a reason for hiding this comment

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

i see what you mean about the resulting ast :) is there a more condensed way to represent it, that is still in line with how everything else is parsed?

@ryandmaggio
Copy link
Author

I'm not sure what is going on with the test failures, I'm still looking into it, but the failures are in parts of the code that I hadn't knowingly touched (malformed if statements).

I'll make sure to squash the commits on the next one (I'm pretty sure there are going to be more changes).

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