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

[Codegen] Refactor the Codegen #228

Merged
merged 90 commits into from
Aug 15, 2024
Merged

[Codegen] Refactor the Codegen #228

merged 90 commits into from
Aug 15, 2024

Conversation

luke-hill
Copy link
Contributor

@luke-hill luke-hill commented Jun 14, 2024

🤔 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?

  • 🏦 Refactoring/debt/DX (improvement to code design, tooling, etc. without changing behaviour)

♻️ 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:

  • I agree to respect and uphold the Cucumber Community Code of Conduct
  • I've changed the behaviour of the code
    • I have added/updated tests to cover my changes.
  • My change requires a change to the documentation.
    • I have updated the documentation accordingly.
  • Users should know about my change
    • I have added an entry to the "Unreleased" section of the CHANGELOG, linking to this pull request.

This text was originally generated from a template, then edited by hand. You can modify the template here.

Sorry, something went wrong.

luke-hill added 30 commits June 14, 2024 11:20

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
Fix up a couple of rogue cops regarding regex
Configure all newcops
#format_description is a long-winded way of doing nothing. So simplify it
@luke-hill
Copy link
Contributor Author

@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.

@clrudolphi
Copy link
Contributor

@luke-hill Thanks for the heads-up. I think we're done, just pending final reviews by @mpkorstanje and @gasparnagy
Will your work finish up in the next day or two? If not, I hope we will slip in ahead of you.

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
Copy link
Contributor Author

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.

Copy link
Member

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.

@luke-hill
Copy link
Contributor Author

Ping @clrudolphi - I've only made some minor changes to the dotNET stuff here, but if you want to check it out go ahead

@clrudolphi
Copy link
Contributor

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.

@luke-hill luke-hill merged commit a8c4419 into main Aug 15, 2024
26 checks passed
@luke-hill luke-hill deleted the feature/refactor_codegen branch August 15, 2024 08:16
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.

None yet

3 participants