event-catalog / generator-asyncapi

AsyncAPI generator for EventCatalog
https://www.eventcatalog.dev/docs/development/integrations/async-api/intro
Other
9 stars 4 forks source link

keep original vs parsed spec / not overwriting existing specs in service #40

Closed XaaXaaX closed 1 month ago

XaaXaaX commented 1 month ago

Motivation

Keep original spec instead of Parsed one via a generator parameter

changeset-bot[bot] commented 1 month ago

🦋 Changeset detected

Latest commit: a19d4ca4834f8649ef444c7d817dab6759ba6ef2

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package | Name | Type | | -------------------------------- | ----- | | @eventcatalog/generator-asyncapi | Major |

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

codingnuclei commented 1 month ago

I wonder if it is worth having keepOriginalSpec as true by default? It not being true makes it a breaking change as the generator used to preserve the contents as is:

https://github.com/event-catalog/generator-asyncapi/compare/eeb5d45..main#diff-a2a171449d862fe29692ce031981047d7ab755ae7f84c707aef80701b3ea0c80L261-R257

Small comment on the name... keepOriginalSpec - to me it suggests that if false the original file will be deleted. How about saveParsedOuput instead?

XaaXaaX commented 1 month ago

saveParsedOuput

I wonder if it is worth having keepOriginalSpec as true by default? It not being true makes it a breaking change as the generator used to preserve the contents as is:

https://github.com/event-catalog/generator-asyncapi/compare/eeb5d45..main#diff-a2a171449d862fe29692ce031981047d7ab755ae7f84c707aef80701b3ea0c80L261-R257

Small comment on the name... keepOriginalSpec - to me it suggests that if false the original file will be deleted. How about saveParsedOuput instead?

Yes @codingnuclei , the actual system automatically persists the parsed version of spec, so keeping the original spec by default sounds a breaking change, i was waiting for some comments on param naming, sounds fair and is more explicit as it describes the existence of an spec but also the existence of a parsed version.

i suggest using saveParsedSpecFile instead of saveParsedOuput as this will be misleading in generator config, i will understand it as , persisting the Service , and events from the spec in the catalog or not ( you see the point? )

XaaXaaX commented 1 month ago

@boyney123 @codingnuclei new commit to resolve #47 but pushed it to the same branch by accident, if it can go together i leave it as is, otherwise , i can do two separate PRs

boyney123 commented 1 month ago

thanks @XaaXaaX , I updated the code, added some generic stuff to the SDK (get specs by service id, which we will need in other plugins too).

Apart from that happy with the code and breaking changes. I'm also going to add more breaking changes into another PR to remove folderID and enforce a service id.

boyney123 commented 1 month ago

Thank you @XaaXaaX !