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

fix: log the name of the message if it is absent #304

Closed
wants to merge 3 commits into from

Conversation

ibishal
Copy link

@ibishal ibishal commented Jun 20, 2024

Description
Added a param messageRuntimeValidation which will log the messageId's if its missing and if false then it will not warn anything(by default it will be true)
Screenshot from 2024-07-02 02-18-41
image

image
image

Related issue(s)
#197

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Welcome to AsyncAPI. Thanks a lot for creating your first pull request. Please check out our contributors guide useful for opening a pull request.
Keep in mind there are also other channels you can use to interact with AsyncAPI community. For more details check out this issue.

@ibishal
Copy link
Author

ibishal commented Jun 20, 2024

I'm only worried for those that do not care about validation, and cannot modify their documents to add identifiers, what in their case. We should probably allow to disable validation?

Hey @derberg i have not added this feature, we need to make changes in asyncapi/generator for it right ?

@derberg
Copy link
Member

derberg commented Jun 24, 2024

@ibishal no need to add anything to generator.

You simply need to introduce "parameter" feature. So let's say it is called messageRuntimeValidation that by default is set to true. If set to true, we add in generated code message validation code, and we also console log info that some message identifiers are missing. And we also need to log "that if you are not able to modify your asyncapi document to add missing IDs, then disable runtime validation logic by passing parameter messageRuntimeValidation set to false"

makes sense?

@ibishal ibishal changed the title fix: log the name of the message if it is absen fix: log the name of the message if it is absent Jun 27, 2024
@ibishal
Copy link
Author

ibishal commented Jul 1, 2024

Done @derberg

Copy link
Member

@derberg derberg left a comment

Choose a reason for hiding this comment

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

I don't think you tested it manually as just looking at the code I see it errors.

you always have to check manually or at least have a good coverage of integration tests

the basic thing that I see is missing:

  • messageRuntimeValidation parameter is not added to package.json to generator config
  • no info in readme

package.json Outdated Show resolved Hide resolved
@ibishal
Copy link
Author

ibishal commented Jul 18, 2024

I don't think you tested it manually as just looking at the code I see it errors.

I have done manual testing for it. you can see it in the sc of pr description

messageRuntimeValidation parameter is not added to package.json to generator config

It was added in the parameter of generator config in package.json

no info in readme

will add it

@ibishal
Copy link
Author

ibishal commented Jul 18, 2024

Do we need integration test here @derberg

Co-authored-by: Lukasz Gornicki <[email protected]>
@ibishal ibishal requested a review from derberg July 18, 2024 18:01
@derberg
Copy link
Member

derberg commented Aug 3, 2024

I think I mixed the issues scope :(

in general idea was that best DX is that user gets a feedback during generation, and not once the template is generated and code build and running

sorry but instead of parameter I think we need generate:before that will do it -> https://www.asyncapi.com/docs/tools/generator/hooks

wdyt?

@ibishal
Copy link
Author

ibishal commented Aug 25, 2024

sorry but instead of parameter I think we need generate:before that will do it -> https://www.asyncapi.com/docs/tools/generator/hooks

Looks Good

@derberg
Copy link
Member

derberg commented Nov 4, 2024

closing, no further activity from the contributor

@derberg derberg closed this Nov 4, 2024
@ibishal
Copy link
Author

ibishal commented Nov 7, 2024

@derberg, sorry from my side

i want to finish it, can we open this?

Copy link
Member

derberg commented Nov 11, 2024

just reopen new PR but updated according to previous feedback

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