-
-
Notifications
You must be signed in to change notification settings - Fork 21
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
[Codegen] Refactor the Codegen #228
Conversation
Fix up a couple of rogue cops regarding regex Configure all newcops
…he main class outside of generate
@clrudolphi this is about 85% complete at the moment. I've got 2 more batches of things to tidy up, but it's essentially almost done. Again as mentioned if there's going to be a close time overlap, I'll hold off on merging this until you've got yours in that way I can fix up the conflicts my end. |
@luke-hill Thanks for the heads-up. I think we're done, just pending final reviews by @mpkorstanje and @gasparnagy |
README.md
Outdated
There are also 2 subdirectories which have the legacy implementation which was largely driven by protobuf. | ||
These are currently not implemented and are in the process of being ported over to the JSON schema protocol | ||
|
||
- .Net |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ping @gasparnagy / @clrudolphi - It might be worth considering at what "point" we want to announce the changes being made by yourselves such that I can change this comment (Or you can), to move it into the above bracket.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would say that until we don't have the json report gen ready, there might be smaller changes needed, so I would announce it after we have it ready on Reqnroll side.
Ping @clrudolphi - I've only made some minor changes to the dotNET stuff here, but if you want to check it out go ahead |
Looks good. |
🤔 What's changed?
Change the codegen to be structured neatly and properly. Instead of just tacked into the jsonschema
It will live in its own TLD. It will also be split into the subsequent classes, cleaned up via rubocop and eventually neatened up to remove and redundancies / sub-optimisations.
NB: This isn't aimed to improve the codegen much. I know the way each language interprets the generated items is different. For now this is just aimed as a "1st Pass" at tidying up.
⚡️ What's your motivation?
Tidy up messages codebase. Improve Codegen so that we can read it and optimise it more down the line
🏷️ What kind of change is this?
♻️ Anything particular you want feedback on?
This can be thought of as an "Internal" Breaking change. But given no-one who is a consumer will see this breaking change (As all codegen code will be identical), it can go out as a patch release
📋 Checklist:
This text was originally generated from a template, then edited by hand. You can modify the template here.