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

msggen: introduce chain of responsibility pattern to make msggen extensible #5216

Merged

Conversation

vincenzopalazzo
Copy link
Collaborator

@vincenzopalazzo vincenzopalazzo commented Apr 23, 2022

This abstraction was easy beceuse @cdecker make a great work

So, when mssgen will be released like a library, the user can create their own generator that extends IGenerator, and put it inside the following configuration

def gen_dotnet(generator_chain: GeneratorChain):
    fname = "<your path>"
    dest = open(fname, "w")
    generator_chain.add_generator(DotnetGenerator(dest))

if __name__ == "__main__":
  service = load_jsonrpc_service(schema_dir="<json schema path>")
  generator_chain = GeneratorChain()
  gen_dotnet(generator_chain, meta)
  generator_chain.generate(service)

Pro:

  • We don't need to trust the user to push code that works in the own repository, but now the user can generate and test the code in the repository that depended on the model that is generated;
  • The user can export this generator like the library and make available the work as a python package.
  • The user is not coupled with a specific library if any are used, but if there is something wrong for a user in the generator implementation he can reimplement the own.

Cons:

  • a new package to maintain, but it is better that maintains X programming language with its own release process

joemphilips added a commit to joemphilips/DotNetLightning that referenced this pull request Apr 24, 2022
This commit adds a new project for calling c-lightning from .NET
in a typesafe way.
Most of the codes are auto-generated from client generator built in
ref: ElementsProject/lightning#5201

It must be updated after ElementsProject/lightning#5216 gets merged.
So that all the generation logics is defined in this repo.
But this is good enough for temporary solution.
@vincenzopalazzo vincenzopalazzo marked this pull request as ready for review April 24, 2022 12:13
@vincenzopalazzo vincenzopalazzo force-pushed the macros/msggen_abstraction branch 2 times, most recently from b139688 to c49dc09 Compare April 24, 2022 12:34
contrib/msggen/msggen/__main__.py Outdated Show resolved Hide resolved
contrib/msggen/msggen/gen/generator.py Outdated Show resolved Hide resolved
contrib/msggen/msggen/gen/generator.py Outdated Show resolved Hide resolved
contrib/msggen/msggen/__main__.py Show resolved Hide resolved
contrib/msggen/msggen/gen/grpc.py Show resolved Hide resolved
contrib/msggen/msggen/gen/__init__.py Show resolved Hide resolved
joemphilips added a commit to joemphilips/DotNetLightning that referenced this pull request Apr 26, 2022
This commit adds a new project for calling c-lightning from .NET
in a typesafe way.
Most of the codes are auto-generated from client generator built in
ref: ElementsProject/lightning#5201

It must be updated after ElementsProject/lightning#5216 gets merged.
So that all the generation logics is defined in this repo.
But this is good enough for temporary solution.
…nsible

Changelog-Added: msggen: introduce chain of responsibility pattern to make msggen extensible

Signed-off-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>
Signed-off-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>
joemphilips added a commit to joemphilips/DotNetLightning that referenced this pull request Apr 28, 2022
This commit adds a new project for calling c-lightning from .NET
in a typesafe way.
Most of the codes are auto-generated from client generator built in
ref: ElementsProject/lightning#5201

It must be updated after ElementsProject/lightning#5216 gets merged.
So that all the generation logics is defined in this repo.
But this is good enough for temporary solution.
joemphilips added a commit to joemphilips/DotNetLightning that referenced this pull request Apr 28, 2022
This commit adds a new project for calling c-lightning from .NET
in a typesafe way.
Most of the codes are auto-generated from client generator built in
ref: ElementsProject/lightning#5201

It must be updated after ElementsProject/lightning#5216 gets merged.
So that all the generation logics is defined in this repo.
But this is good enough for temporary solution.
joemphilips added a commit to joemphilips/DotNetLightning that referenced this pull request Apr 28, 2022
This commit adds a new project for calling c-lightning from .NET
in a typesafe way.
Most of the codes are auto-generated from client generator built in
ref: ElementsProject/lightning#5201

It must be updated after ElementsProject/lightning#5216 gets merged.
So that all the generation logics is defined in this repo.
But this is good enough for temporary solution.
joemphilips added a commit to joemphilips/DotNetLightning that referenced this pull request Apr 28, 2022
This commit adds a new project for calling c-lightning from .NET
in a typesafe way.
Most of the codes are auto-generated from client generator built in
ref: ElementsProject/lightning#5201

It must be updated after ElementsProject/lightning#5216 gets merged.
So that all the generation logics is defined in this repo.
But this is good enough for temporary solution.
joemphilips added a commit to joemphilips/DotNetLightning that referenced this pull request Apr 28, 2022
This commit adds a new project for calling c-lightning from .NET
in a typesafe way.
Most of the codes are auto-generated from client generator built in
ref: ElementsProject/lightning#5201

It must be updated after ElementsProject/lightning#5216 gets merged.
So that all the generation logics is defined in this repo.
But this is good enough for temporary solution.
joemphilips added a commit to joemphilips/DotNetLightning that referenced this pull request May 2, 2022
This commit adds a new project for calling c-lightning from .NET
in a typesafe way.
Most of the codes are auto-generated from client generator built in
ref: ElementsProject/lightning#5201

It must be updated after ElementsProject/lightning#5216 gets merged.
So that all the generation logics is defined in this repo.
But this is good enough for temporary solution.
@cdecker
Copy link
Member

cdecker commented May 3, 2022

I'm not quite sure what you're trying to achieve here, if we release msggen as a library, then it is trivial to extend the existing classes, and write a tiny entrypoint that loads and generates. There is no need to wire everything into a single top-level executable which seems to be what you're doing here (and I have done myself as a shorthand).

Let's keep the refactoring and the creation of a top-level interface, and drop the plugin-like system of adding a generator to a set of generators that are to be executed as a block. That is also much more in line with the Makefile semantics.

@vincenzopalazzo
Copy link
Collaborator Author

I'm not quite sure what you're trying to achieve here, if we release msggen as a library, then it is trivial to extend the existing classes, and write a tiny entrypoint that loads and generates. There is no need to wire everything into a single top-level executable which seems to be what you're doing here (and I have done myself as a shorthand).

The chain of responsibility IMHO is a good addition to include in the library, because can be some cases where we can dependencies between steps.

This pattern can make the code cleaner to read and write imho. However, the same solution is achieved with the hardcoded inside the python script from the user!

joemphilips added a commit to joemphilips/DotNetLightning that referenced this pull request May 5, 2022
This commit adds a new project for calling c-lightning from .NET
in a typesafe way.
Most of the codes are auto-generated from client generator built in
ref: ElementsProject/lightning#5201

It must be updated after ElementsProject/lightning#5216 gets merged.
So that all the generation logics is defined in this repo.
But this is good enough for temporary solution.
@cdecker
Copy link
Member

cdecker commented May 5, 2022

I see, the generator chain threw me off, as it looked like we'd end up creating a single huge chain that'd add all the generators to a single entry-point, whereas you plan to build a chain for each entry-point which can then live in various different repos, independently of this repo.

Seems quite alright, thanks for the cleanup along the way. I'm not exactly convinced that the chain of responsibility pattern is what we want here (or if it is even achieved by the code, since every message goes through every generator, and isn't handled by the first matching generator), but it doesn't add much complexity and can serve as a grouping of logically correlated generators.

ACK 79c3df7

joemphilips added a commit to joemphilips/DotNetLightning that referenced this pull request May 6, 2022
This commit adds a new project for calling c-lightning from .NET
in a typesafe way.
Most of the codes are auto-generated from client generator built in
ref: ElementsProject/lightning#5201

It must be updated after ElementsProject/lightning#5216 gets merged.
So that all the generation logics is defined in this repo.
But this is good enough for temporary solution.
@cdecker cdecker merged commit 2ab2061 into ElementsProject:master May 7, 2022
@vincenzopalazzo vincenzopalazzo deleted the macros/msggen_abstraction branch May 17, 2022 19:53
joemphilips added a commit to joemphilips/DotNetLightning that referenced this pull request May 18, 2022
This commit adds a new project for calling c-lightning from .NET
in a typesafe way.
Most of the codes are auto-generated from client generator built in
ref: ElementsProject/lightning#5201

It must be updated after ElementsProject/lightning#5216 gets merged.
So that all the generation logics is defined in this repo.
But this is good enough for temporary solution.
joemphilips added a commit to joemphilips/DotNetLightning that referenced this pull request May 19, 2022
This commit adds a new project for calling c-lightning from .NET
in a typesafe way.
Most of the codes are auto-generated from client generator built in
ref: ElementsProject/lightning#5201

It must be updated after ElementsProject/lightning#5216 gets merged.
So that all the generation logics is defined in this repo.
But this is good enough for temporary solution.
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