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

Add ApplicationData datatype #485

Merged
merged 2 commits into from
Oct 5, 2024
Merged

Conversation

yuanchen233
Copy link
Collaborator

As discussed in #484, introducing a new datatype to hold applicationDataand set default serializer to ApplicationDataSerializer. This way we don't need to specify the serializer used everytime we use applicationData.

Changes:

  1. New type ApplicationData holds a String field data.
  2. In ApplicationDataSerializer, a not null String value is expected for field data
  3. All applicationData are now type of ApplicationData?
  4. Update TypeScript wrapper

@yuanchen233 yuanchen233 marked this pull request as ready for review October 4, 2024 13:45
@yuanchen233 yuanchen233 requested a review from Whathecode October 4, 2024 13:45
Copy link
Member

@Whathecode Whathecode left a comment

Choose a reason for hiding this comment

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

The changes look good!

However, I figured this should target main, so this fix is available prior to your other PR (which otherwise still has a bugged commit).

Or, you fix the other PR by adding the custom serializer on the String?, which then in this PR gets removed again. But, I tend to avoid that type of multiple modifications if better prerequisites are known. 🙂

I can rebase/move stuff for you if you want. Other than code style and commit order, I think these PRs are good to be merged.

@yuanchen233
Copy link
Collaborator Author

yuanchen233 commented Oct 5, 2024

Sounds good.

I can rebase/move stuff for you if you want. Other than code style and commit order, I think these PRs are good to be merged.

Yes please, and thanks. I try to avoid doing interactive rebase here (at least for now), as there is a chance I mess up everything and creates more work for both of us. 🙂

@Whathecode Whathecode changed the base branch from extra-information-for-device-registration to develop October 5, 2024 13:29
@Whathecode Whathecode force-pushed the add-application-data-datatype branch from b4a6d3d to 28d5e3a Compare October 5, 2024 21:53
This way, the serializer doesn't need to be specified on every field which requires `ApplicationDataSerializer`. Specifically, in case of inheritance, this prevents having to apply the serializer to all inheriting classes on base properties which need it.
@Whathecode Whathecode force-pushed the add-application-data-datatype branch from 28d5e3a to 77f9bf0 Compare October 5, 2024 22:33
Copy link
Member

@Whathecode Whathecode left a comment

Choose a reason for hiding this comment

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

I just rebased on develop and removed the use of ApplicationData in the commits which aren't yet merged to the develop branch.

I also slightly reworked the ApplicationData and ApplicationDataSerializer documentation to avoid redundancies. The type now already documents the purpose, so no real need to reiterate on that fully on the serializer.

@Whathecode Whathecode merged commit 94664ce into develop Oct 5, 2024
3 checks passed
@Whathecode Whathecode deleted the add-application-data-datatype branch October 5, 2024 22:47
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