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

Model builder fix #78

Merged
merged 25 commits into from
Oct 26, 2018
Merged

Model builder fix #78

merged 25 commits into from
Oct 26, 2018

Conversation

Victorious3
Copy link
Collaborator

@Victorious3 Victorious3 commented Aug 30, 2018

Fixes #70

  • Multiple base classes aren't carried over to the generated python code, instead @tatsumasu only gets the first one (without any ::) This is intentional
  • synthesize was receiving string parameters as base classes instead of type objects
  • It's unclear what is going to happen when multiple base classes are specified

It might be a good idea to synthesize classes with a directive, i.e something like

@@model :: B(C): a, b   # B is subclass of C and has fields a and b
@@model :: A(B): c      # A is subclass of B and has field c (inherits a, b)
@@model :: D(E, F): a   # D is subclass of E and F (multiple inheritance)

Then only one argument to the rules would be accepted

rule::D = ();  # D does already inherit E and F, no need to specify it here

With these changes the model could be part of the generated parser as well.

I also found that the generated model (python source code) isn't of much use if you rely on the base classes being defined elsewhere. It would need a parameter to add an import statement or some other way to inject them (importlib). - This is now part of the PR

Changes

  • There is now an extra parameter base_type for to_python_model which corresponds to the one you can pass to ModelBuilderSemantics, this feature was previously unavailable for generated models
  • Tatsu's CLI got an extra parameter --base-type that exposes the above
  • The generated model builder gets two optional parameters, types (takes precedence over generated types if specified) and context that are passed on
  • You can now safely specify a base class inside your grammar using rule::Class::BaseClass

@Victorious3
Copy link
Collaborator Author

The Model Renderer only accepts one base class and ignores the others, where the one that synthesizes classes on the fly would (if it didn't error out) deal with multiple base classes. This is inconsistent. @apalala What was the intention here?

@Victorious3
Copy link
Collaborator Author

I've decided to go for single inheritance now, i.e
A::B::C means "A is a subclass of B and B is a subclass of C"

@Victorious3
Copy link
Collaborator Author

I'm going to let it generate the base classes as well, then this should be ready for merge.

@apalala
Copy link
Collaborator

apalala commented Oct 26, 2018

@Victorious3 This is a complex set of changes, so I won't review them in detail. I'll test for regressions, though:

  1. make test
  2. run tests for complex Grako projects ported to TatSu, like COBOL.

Copy link
Collaborator

@apalala apalala left a comment

Choose a reason for hiding this comment

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

LGTM!

@apalala apalala merged commit 07cdb5e into neogeny:master Oct 26, 2018
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