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

Move AST to separate package #180

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

Move AST to separate package #180

wants to merge 10 commits into from

Conversation

riz0id
Copy link
Collaborator

@riz0id riz0id commented Feb 13, 2022

This PR isolates Proto3.Suite.DotProto.AST into separate package proto3-suite-ast.

@riz0id riz0id self-assigned this Feb 13, 2022
@riz0id riz0id linked an issue Feb 13, 2022 that may be closed by this pull request
@Gabriella439
Copy link
Contributor

Gabriella439 commented Feb 14, 2022

The original request for this was from quite a while ago so I wanted to check if we still need to split this out into a separate package?

I do think there is a good reason to split into multiple packages, but for a different reason: I think there should be a package that implements the code generation logic and the generated code should depend on a separate package. However, I don't believe separating out the AST into a separate package addresses that.

@riz0id
Copy link
Collaborator Author

riz0id commented Feb 14, 2022

I can't think of a reason why the syntax datatype would be useful in a separate package even if I went ahead and added the parser into the new package. I don't mind removing the new package, but I do think that some of the changes in this PR like documentation, removing orphan instances out of Proto3.Suite.DotProto.Rendering, and the new modules compartmentalizing things a bit more are useful though.

@riz0id riz0id marked this pull request as draft February 14, 2022 21:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

AST to seperate package
3 participants