refactor: add nullable information

[jonaslagoni]

Description This PR adds isNullable property to determine if models should be nullable. It does not go into depth in regards to what exactly all possible nullable types are, that will be another PR, here I only introduce the bare minimum based on the current implementation.

For Java, this means using specific wrapper types such as Integer, Float, and Boolean. For TS, this means rendered types as a union with null T | null. For Go, this means using pointers. For C#, this means using ?.

[jeremylvln]

Hi! This looks really promising. Does this PR supersede mine #755?

[jonaslagoni]

Hi! This looks really promising. Does this PR supersede mine #755?

Hey @IamBlueSlime not at all, this pr targets the next branch, which introduce a new core model.

Your contribution and #752 is what this change is based on.

The reason why I have not reviewed your PRs is because I cant wrap my head around the best approach in the old setup 🙃 I still have not decided on the next steps for the PRs.

Regardless, this pr will be merged with you and @robingoussey as contributors to this change 🙂

[jonaslagoni]

Can you add a test for ['string', 'number', 'null'] for all languages? This is a pretty important case.

Good call, added a test for ensuring union model are created accurately: https://github.com/asyncapi/modelina/pull/759/commits/abb61157824f7991eeaeabfe208929828381bdc0

This will work regardless of language 🙂

[sonarcloud[bot]]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
No Duplication information

[jeremylvln]

Hi! This looks really promising. Does this PR supersede mine #755?

Hey @IamBlueSlime not at all, this pr targets the next branch, which introduce a new core model.

Your contribution and #752 is what this change is based on.

The reason why I have not reviewed your PRs is because I cant wrap my head around the best approach in the old setup 🙃 I still have not decided on the next steps for the PRs.

Regardless, this pr will be merged with you and @robingoussey as contributors to this change 🙂

Got it. I agree with you for the architecture of the old setup.

If I was you, I would make a trade-off: if the goal is to release the next branch ASAP, don't bother much during the review. If a PR is good enough and does not prevent further improvements, go on. Unfortunately, maintaining legacy is making compromises 😔

This change is the only one preventing me from using Modelina in my company's workflow. Do you think next is safe enough to use it?

Thanks for your work!

[magicmatatjahu]

@jonaslagoni

Good call, added a test for ensuring union model are created accurately: https://github.com/asyncapi/modelina/commit/abb61157824f7991eeaeabfe208929828381bdc0

This will work regardless of language 🙂

Thanks! However, we should also test how that case it will rendered for languages. From code perspective I guess that we can have a problem and it will render as number | null | string | null, although I could be wrong.

[jonaslagoni]

Thanks! However, we should also test how that case it will rendered for languages. From code perspective I guess that we can have a problem and it will render as number | null | string | null, although I could be wrong.

No, you are right, it will render it as so, but it's valid syntax 🙂

[magicmatatjahu]

@jonaslagoni What about other languages? Won't the types be duplicated? Maybe treat null as a separate "object", the same way we treat Float Integer etc.

[jonaslagoni]

@jonaslagoni What about other languages? Won't the types be duplicated? Maybe treat null as a separate "object", the same way we treat Float Integer etc.

Hmm, let me give it some thoughts 🤔

[jonaslagoni]

This change is the only one preventing me from using Modelina in my company's workflow. Do you think next is safe enough to use it?

@IamBlueSlime not yet no, it is still in the middle of the rewriting to the new model.

I have added a review for your PR 🙂

[github-actions[bot]]

This pull request has been automatically marked as stale because it has not had recent activity :sleeping:

It will be closed in 120 days if no further activity occurs. To unstale this pull request, add a comment with detailed explanation.

There can be many reasons why some specific pull request has no activity. The most probable cause is lack of time, not lack of interest. AsyncAPI Initiative is a Linux Foundation project not owned by a single for-profit company. It is a community-driven initiative ruled under open governance model.

Let us figure out together how to push this pull request forward. Connect with us through one of many communication channels we established here.

Thank you for your patience :heart:

[sonarcloud[bot]]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
No Duplication information

[jonaslagoni]

We are gonna re-create the next branch for version 2 changes, which will close your PR, feel free to re-create or retarget your PR for master as it contains the version 1 changes 🙂 Or if your change contains a breaking change, target it for the version 2 in next.