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

feat!: properly implement constants in c# #1801

Merged
merged 8 commits into from
Feb 19, 2024

Conversation

moritzkalwa
Copy link
Contributor

@moritzkalwa moritzkalwa commented Feb 13, 2024

Description

Enable the C# Class and Record Generators to properly handle constant values.

Related Issue

fixes #1471

Checklist

  • The code follows the project's coding standards and is properly linted (npm run lint).
  • Tests have been added or updated to cover the changes.
  • Documentation has been updated to reflect the changes.
  • All tests pass successfully locally.(npm run test).

Copy link

netlify bot commented Feb 13, 2024

Deploy Preview for modelina canceled.

Name Link
🔨 Latest commit b6a4f2f
🔍 Latest deploy log https://app.netlify.com/sites/modelina/deploys/65d21ac6b62e740008a1b130

@moritzkalwa moritzkalwa changed the title Constants are compiled as enums feat: properly implement constants in c# Feb 13, 2024
@coveralls
Copy link

coveralls commented Feb 14, 2024

Pull Request Test Coverage Report for Build 7913235475

Details

  • -9 of 24 (62.5%) changed or added relevant lines in 3 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage decreased (-0.2%) to 92.168%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/generators/csharp/renderers/ClassRenderer.ts 4 6 66.67%
src/generators/csharp/constrainer/ConstantConstrainer.ts 9 16 56.25%
Files with Coverage Reduction New Missed Lines %
src/generators/csharp/constrainer/ConstantConstrainer.ts 1 50.0%
Totals Coverage Status
Change from base Build 7913190707: -0.2%
Covered Lines: 6007
Relevant Lines: 6347

💛 - Coveralls

@jonaslagoni
Copy link
Member

jonaslagoni commented Feb 14, 2024

I think we have to do this change on the next branch, as the serializer currently in place will break as they will try assign values to the constants 🤔 I think that either we add an issue after this PR is merged, or we adapt the presets where needed.

On top of that it will break people's existing usage of the models.

@moritzkalwa do you mind adding an entry in the migration guide ✌️

@jonaslagoni jonaslagoni changed the title feat: properly implement constants in c# feat!: properly implement constants in c# Feb 14, 2024
@jonaslagoni
Copy link
Member

Similar to #1799

@moritzkalwa
Copy link
Contributor Author

@jonaslagoni No, I can definitely add a migration guide for this. I probably won't have the time for it until Friday though.

@jonaslagoni
Copy link
Member

@jonaslagoni No, I can definitely add a migration guide for this. I probably won't have the time for it until Friday though.

No worries, no rush ✌️

@jonaslagoni jonaslagoni changed the base branch from master to next February 15, 2024 08:41
@jonaslagoni jonaslagoni changed the base branch from next to master February 15, 2024 08:41
@jonaslagoni jonaslagoni changed the base branch from master to next February 15, 2024 09:11
@jonaslagoni
Copy link
Member

Changed the target branch to next ✌️

@moritzkalwa
Copy link
Contributor Author

I think we have to do this change on the next branch, as the serializer currently in place will break as they will try assign values to the constants 🤔 I think that either we add an issue after this PR is merged, or we adapt the presets where needed.

If you point me to the correct place I might be able to adapt the serializers right in this PR / add a second PR before we merge this one?

@jonaslagoni
Copy link
Member

You have the presets here: https://github.com/asyncapi/modelina/blob/master/src/generators/csharp/presets/JsonSerializerPreset and https://github.com/asyncapi/modelina/blob/master/src/generators/csharp/presets/NewtonsoftSerializerPreset.ts

I think you only need to make changes to renderSerializeProperty and renderDeserializeProperty and check whether the model has a constant value, if so, dont serialize what you get, just set it 🙂

@moritzkalwa
Copy link
Contributor Author

You have the presets here: https://github.com/asyncapi/modelina/blob/master/src/generators/csharp/presets/JsonSerializerPreset and https://github.com/asyncapi/modelina/blob/master/src/generators/csharp/presets/NewtonsoftSerializerPreset.ts

I think you only need to make changes to renderSerializeProperty and renderDeserializeProperty and check whether the model has a constant value, if so, dont serialize what you get, just set it 🙂

Im still trying to understand the serializer so forgive me if this is a dumb question, but do we really need to change the renderDeserializeProperty function? Writing to the constant values would be a problem for sure, but deserialising shouldn't be a problem, because it only reads that property, right?

@jonaslagoni
Copy link
Member

Most likely correct @moritzkalwa 👍 you can use the runtime test to ensure it works 😄

Copy link

sonarcloud bot commented Feb 18, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues

Measures
0 Security Hotspots
No data about Coverage
1.5% Duplication on New Code

See analysis details on SonarCloud

Copy link
Member

@jonaslagoni jonaslagoni left a comment

Choose a reason for hiding this comment

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

Amazing @moritzkalwa 🔥

@jonaslagoni
Copy link
Member

/rtm

@asyncapi-bot asyncapi-bot merged commit 225f730 into asyncapi:next Feb 19, 2024
17 checks passed
@asyncapi-bot
Copy link
Contributor

🎉 This PR is included in version 4.0.0-next.4 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Constants are compiled as enums
4 participants