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: add support for name based serialisation of JMS enums #2355

Merged
merged 1 commit into from
Oct 31, 2024

Conversation

bobvandevijver
Copy link
Contributor

@bobvandevijver bobvandevijver commented Oct 11, 2024

Q A
Bug fix? no
New feature? yes
Deprecations? no
Issues -

This PR add a method to detect when the developer has annotated their property with a JMS type that specifies that the backed enum needs to be serialized with its name instead of value. I needed to use the serialization context as the model options are not included in the model hash, while I do need to generate a new unique description. I've chosen to append Name to the resulting Model name.

Depends on #2372

@bobvandevijver bobvandevijver changed the title Improve JMS enum handling fix: Improve JMS enum handling Oct 11, 2024
@bobvandevijver bobvandevijver changed the title fix: Improve JMS enum handling fix: improve JMS enum handling Oct 11, 2024
@bobvandevijver bobvandevijver force-pushed the enum-handling branch 5 times, most recently from 540b3cf to 73c02ce Compare October 11, 2024 13:30
Copy link

codecov bot commented Oct 25, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.37%. Comparing base (1e283ef) to head (f90f083).
Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2355      +/-   ##
==========================================
+ Coverage   89.36%   89.37%   +0.01%     
==========================================
  Files          77       77              
  Lines        2849     2853       +4     
==========================================
+ Hits         2546     2550       +4     
  Misses        303      303              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@bobvandevijver
Copy link
Contributor Author

@DjordyKoert With JMS 3.31 out this change is required for this bundle to be able to work with that version of JMS.

@DjordyKoert
Copy link
Collaborator

@DjordyKoert With JMS 3.31 out this change is required for this bundle to be able to work with that version of JMS.

Would it make sense to split this PR?
One to fix schmittjoh/serializer#1561 and one to introduce the new feature. Makes it easier to review & to find the individual fix / feature in the commit history

@bobvandevijver bobvandevijver changed the title fix: improve JMS enum handling feat: add support for name based serialisation of JMS enums Oct 31, 2024
@bobvandevijver
Copy link
Contributor Author

@DjordyKoert With JMS 3.31 out this change is required for this bundle to be able to work with that version of JMS.

Would it make sense to split this PR? One to fix schmittjoh/serializer#1561 and one to introduce the new feature. Makes it easier to review & to find the individual fix / feature in the commit history

I splitted it on commit level for that purpose, but makes sense to split it on PR level as well. I've created #2372 for the fix, and updated this PR to only contain the new feature.

Removing the fix commit from this PR results in a merge conflict, so I will do that once the new PR has been handled.

@DjordyKoert
Copy link
Collaborator

@DjordyKoert With JMS 3.31 out this change is required for this bundle to be able to work with that version of JMS.

Would it make sense to split this PR? One to fix schmittjoh/serializer#1561 and one to introduce the new feature. Makes it easier to review & to find the individual fix / feature in the commit history

I splitted it on commit level for that purpose, but makes sense to split it on PR level as well. I've created #2372 for the fix, and updated this PR to only contain the new feature.

Removing the fix commit from this PR results in a merge conflict, so I will do that once the new PR has been handled.

#2372 has been merged :)

@bobvandevijver bobvandevijver force-pushed the enum-handling branch 3 times, most recently from 0237c9d to b0af8a0 Compare October 31, 2024 11:53
@bobvandevijver
Copy link
Contributor Author

Updates have been done, should be ready 😃

@DjordyKoert DjordyKoert merged commit 9ee5f58 into nelmio:master Oct 31, 2024
18 checks passed
@bobvandevijver bobvandevijver deleted the enum-handling branch October 31, 2024 12:11
@DjordyKoert
Copy link
Collaborator

Thank you 😄

DjordyKoert pushed a commit that referenced this pull request Oct 31, 2024
| Q | A |

|---------------|---------------------------------------------------------------------------------------------------------------------------|
| Bug fix? | no |
| New feature? | yes <!-- please update src/**/CHANGELOG.md files --> |
| Deprecations? | no <!-- please update UPGRADE-*.md and
src/**/CHANGELOG.md files --> |
| Issues | - <!-- prefix each issue number with "Fix #", no need to
create an issue if none exists, explain below instead --> |

<!--
Replace this notice by a description of your feature/bugfix.
This will help reviewers and should be a good start for the
documentation.

Additionally:
 - Always add tests and ensure they pass.
- For new features, provide some code snippets to help understand usage.
-->

This PR add a method to detect when the developer has annotated their
property with a JMS type that specifies that the backed enum needs to be
serialized with its name instead of value. I needed to use the
serialization context as the model options are not included in the model
hash, while I do need to generate a new unique description. I've chosen
to append `Name` to the resulting Model name.

Depends on #2372

(cherry picked from commit 9ee5f58)
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