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

LEGO DTO #221

Merged
merged 33 commits into from
Aug 12, 2024
Merged

LEGO DTO #221

merged 33 commits into from
Aug 12, 2024

Conversation

yurvon-screamo
Copy link
Collaborator

@yurvon-screamo yurvon-screamo commented Jul 8, 2024

Description

  • All existing models of the AsyncApi circuit have been replaced with Lego library
  • Test fixed
  • Docs fixed
  • Samples fixed
  • A migration guide has been written
  • Test referense handle

----- additional q

  • Protobuf test
  • Ref + component on model

Related issue(s)

#188

@yurvon-screamo
Copy link
Collaborator Author

yurvon-screamo commented Jul 19, 2024

Hi @VisualBean , I've finished this, please take a look.

Most of all, I am concerned about the correctness of the build of the message payload, I did not understand this contract well in places.

There is also a big disadvantage in this implementation - I do not use the component section at all, I do not yet understand what problems this can turn into, but I guess it's not good...

Please see how the time is going to be)

@VisualBean
Copy link
Collaborator

VisualBean commented Jul 19, 2024

Hi @VisualBean , I've finished this, please take a look.

Most of all, I am concerned about the correctness of the build of the message payload, I did not understand this contract well in places.

There is also a big disadvantage in this implementation - I do not use the component section at all, I do not yet understand what problems this can turn into, but I guess it's not good...

Please see how the time is going to be)

Looks great! Ive only done a preliminary review on my phone (currently on holidays).
But in terms of using components, if you use a reference, it must exist in components (i believe i saw this for the bindings for instance.

Components are however not necesary if you just inline everything. (Whether people want that or not is however a cause for concern on this)

For the message payload, in the current version of asyncapi.net, there is only support for jsonschema, however Avro is stewing in vNext (until i get external reference resolution to work).

Ill have s closer look when im back next week.

@yurvon-screamo
Copy link
Collaborator Author

Hi @VisualBean , I've finished this, please take a look.
Most of all, I am concerned about the correctness of the build of the message payload, I did not understand this contract well in places.
There is also a big disadvantage in this implementation - I do not use the component section at all, I do not yet understand what problems this can turn into, but I guess it's not good...
Please see how the time is going to be)

Looks great! Ive only done a preliminary review on my phone (currently on holidays). But in terms of using components, if you use a reference, it must exist in components (i believe i saw this for the bindings for instance.

Components are however not necesary if you just inline everything. (Whether people want that or not is however a cause for concern on this)

For the message payload, in the current version of asyncapi.net, there is only support for jsonschema, however Avro is stewing in vNext (until i get external reference resolution to work).

Ill have s closer look when im back next week.

All right, I'll be waiting!

I've never worked with Avro, I can't tell you anything here... But this gave me the idea to check how all this will work on protobuf, I'll do it for now)

@VisualBean
Copy link
Collaborator

VisualBean commented Jul 19, 2024

What i did for Avro is basically just the classes and turning message.payload into an IMessagePayload.
AsyncapiSchema then inherits from this, and so does the Avro thing.
(Avro is fairly complex from a parsing standpoint though as it doesnt have a top level object so to speak.
But thats not what this PR is about 😁

But in terms of refs and components.
All classes that inherits from IAsyncApiReferenceable will have the ability to be a reference.

So either its created as a reference (id and type -> which corresponds to a placement in the components section) (i.e new AsyncApiChannel() { Reference = new AsyncApiReference... }) or they have the data themselves.

If it has a reference, itll be looked up and inlined (depending on Writer and Reader settings) in the final document.

Copy link
Collaborator

@VisualBean VisualBean left a comment

Choose a reason for hiding this comment

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

I am really missing a "From this setup using Saunter, we expect this document to be produced" kind of test.
or maybe i missed it?

Otherwise everything looks good i think - cant immediately spot anything.

What about bindings?
Are these passed through the annotations?

The conflict should be easy as all of those files have been removed.

@yurvon-screamo
Copy link
Collaborator Author

yurvon-screamo commented Aug 7, 2024

I am really missing a "From this setup using Saunter, we expect this document to be produced" kind of test. or maybe i missed it?

Otherwise everything looks good i think - cant immediately spot anything.

What about bindings? Are these passed through the annotations?

The conflict should be easy as all of those files have been removed.

after this pull request, i will be implementing integration tests for the project, this will be one of them)

the lego package with bindings is connected, I will add a test that will check them + eliminate conflicts. I'll do it today or tomorrow

@yurvon-screamo
Copy link
Collaborator Author

I am really missing a "From this setup using Saunter, we expect this document to be produced" kind of test. or maybe i missed it?

Otherwise everything looks good i think - cant immediately spot anything.

What about bindings? Are these passed through the annotations?

The conflict should be easy as all of those files have been removed.

done:

  • add bindings to example project
  • add unit test with bindings
  • add docs about bindings

@yurvon-screamo yurvon-screamo merged commit 0461df2 into main Aug 12, 2024
3 checks passed
@yurvon-screamo yurvon-screamo deleted the feature/dto-from-AsyncApi.Net branch August 12, 2024 12:07
@yurvon-screamo
Copy link
Collaborator Author

@VisualBean Can we release it? It seems to me that we have already collected a lot of changes and it's time

@VisualBean
Copy link
Collaborator

@VisualBean Can we release it? It seems to me that we have already collected a lot of changes and it's time

You said something about following up with tests.
I really want to ensure that the output is the same between the old version and the new.
Or if not the same, is 'more' correct. Before we release this major overhaul

@yurvon-screamo
Copy link
Collaborator Author

@VisualBean Can we release it? It seems to me that we have already collected a lot of changes and it's time

You said something about following up with tests. I really want to ensure that the output is the same between the old version and the new. Or if not the same, is 'more' correct. Before we release this major overhaul

Yes, I plan to create a comprehensive test model of the project and implement it.

Now then I'll create an issue and start working on it)

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