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.

Sign up for GitHub

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

Jump to bottom

refactor: enable Python when grammar has 4 sections #111

Merged
merged 1 commit into from
Apr 20, 2024
Merged

refactor: enable Python when grammar has 4 sections #111

merged 1 commit into from
Apr 20, 2024

Conversation

StoneyJackson
Copy link
Member

This is in anticipation of a new hybrid Java/Python mode in which the parser runs in Java and the semantics rep-loop runs in Python.

In this commit, when an optional 4th section is detected, plcc.py generates ParseJsonAst.json. Thus allowing the parser to to generate JSON ASTs when the --json_ast flag is passed. It also generates experimental Python files.

Closes #95

@StoneyJackson
Copy link
Member Author

@reedeveris @AksharP5

Yesterday, after we merged #96, @AksharP5 noticed that one of the ourPLCC/language's tests fails. I started to investigate and noticed that the new commit undid a couple of the new commits that were merged into main, including a script to run the ourPLCC/language's tests in ourPLCC/plcc.

Because of these problems, I have created a new python branch and it is associated with this merge request. Then I reset main to the previous merge commit, and force pushed it (so make sure you don't reuse an existing GItPod workspace; get a fresh one).

So at this point, this PR is challenged and may not get merged if we can't adequately address the problems.

Personally, I think the best approach would be to reproduce the work in this PR in a sequence of smaller PRs that start from main. Since you already solved the hard problems, this should be relatively "easy" and instead you can focus on making each change tight, clean, simple, and obvious. You could try a different approach, but I think this approach has the greatest chance of success.

@AksharP5
Copy link
Collaborator

This is the error that SET was throwing with the latest version of PLCC
image

@reedeveris
Copy link
Collaborator

@StoneyJackson
Please review for merge
I have fixed the problem with the SET language test, so all language tests pass now.
I have fixed the errant commits and resquashed, and retested everything after squashing and pushing, so I believe this is ready to merge again.

StoneyJackson
StoneyJackson commented Apr 15, 2024
View reviewed changes
Copy link
Member Author

@StoneyJackson StoneyJackson left a comment

Choose a reason for hiding this comment

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

My main concern is hard-coding indentation rather than using the provided indent function. If we decide that we later want to use spaces instead of tabs, we'll have to track down every line with \t, assess whether that \t represents indentation or has some other purpose, and then make the change.

src/plcc.py Outdated
}}
""".format(cls=cls,
base=base,
ext=ext,
cases='\n'.join(indent(2, caseList))
cases='\n\t\t\t'.join(caseList)
Copy link
Member Author

Choose a reason for hiding this comment

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

Why not use indent() as before? I'm suspicious of hardcoding tabs here when there was a nice function that added the indents before.

src/plcc.py Outdated
""".format(cls=cls,
base=base,
ext=ext,
cases='\n\t\t\t'.join(caseList)
Copy link
Member Author

Choose a reason for hiding this comment

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

Ditto.

src/plcc.py Outdated
}}
""".format(cls=cls,
lhs=lhs,
ext=ext,
ruleString=ruleString,
decls='\n'.join(indent(1, decls)),
decls='\t'.join(decls),
Copy link
Member Author

Choose a reason for hiding this comment

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

ditto

src/plcc.py Outdated
params=', '.join(params),
inits='\n'.join(indent(2, inits)),
inits='\n\t\t'.join(inits),
Copy link
Member Author

Choose a reason for hiding this comment

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

ditto

src/plcc.py Outdated
lhs=lhs,
ext=ext,
ruleString=ruleString,
decls='\n\t'.join(decls),
Copy link
Member Author

Choose a reason for hiding this comment

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

ditto

src/plcc.py Outdated
if mod and mod != 'ignore' and mod != 'top':
codeString = '\n\t\t'.join(codeString)
else:
codeString = '\n\t'.join(codeString)
Copy link
Member Author

Choose a reason for hiding this comment

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

ditto

src/plcc.py Outdated
@@ -959,18 +1093,19 @@ def sem(nxt):
stub = stub.replace(repl, '{} {}'.format(codeString,repl))
else: # the default
repl = '//{}//'.format(cls)
stub = stub.replace(repl, '{}\n\n{}'.format(codeString,repl))
stub = stub.replace(repl, '{}\n\n\t{}'.format(codeString,repl))
Copy link
Member Author

Choose a reason for hiding this comment

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

ditto

src/plcc.py Outdated
mod = mod.strip() # mod might be 'import', 'top', etc.
if cls in python_stubs:
if mod and mod != 'ignore' and mod != 'top':
codeString = '\n\t\t'.join(codeString)
Copy link
Member Author

Choose a reason for hiding this comment

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

ditto

src/plcc.py Outdated
if mod and mod != 'ignore' and mod != 'top':
codeString = '\n\t\t'.join(codeString)
else:
codeString = '\n\t'.join(codeString)
Copy link
Member Author

Choose a reason for hiding this comment

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

ditto

src/plcc.py Outdated
stub = stub.replace(repl, '{} {}'.format(codeString,repl))
else: # the default
repl = '#{}#'.format(cls)
stub = stub.replace(repl, '{}\n\n\t{}'.format(codeString,repl))
Copy link
Member Author

Choose a reason for hiding this comment

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

ditto

@StoneyJackson @reedeveris @AksharP5
refactor: enable Python when grammar has 4 sections
be3aa2e 
This is in anticipation of a new hybrid Java/Python mode
in which the parser runs in Java and the semantics rep-loop
runs in Python.

In this commit, when an optional 4th section is detected,
plcc.py generates ParseJsonAst.json. Thus allowing the
parser to to generate JSON ASTs when the `--json_ast`
flag is passed. It also generates experimental Python files.

---

Closes #95

Co-authored-by: Reed Everis <[email protected]>
Co-authored-by: Akshar Patel <[email protected]>
Co-authored-by: Stoney Jackson <[email protected]>
@StoneyJackson
Copy link
Member Author

Good work. Thanks @reedeveris @AksharP5 !

@StoneyJackson StoneyJackson merged commit dd27f16 into main Apr 20, 2024
4 checks passed
@StoneyJackson StoneyJackson deleted the python branch April 20, 2024 00:57
@github-actions GitHub Actions
Copy link

🎉 This PR is included in version 6.2.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
released
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Enable JSON ASTs when 4th grammar section detected
3 participants
@StoneyJackson @AksharP5 @reedeveris