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

Make deviceDisplayName optional in serialized DeviceRegistration #486

Closed
Whathecode opened this issue Oct 6, 2024 · 1 comment · Fixed by #487
Closed

Make deviceDisplayName optional in serialized DeviceRegistration #486

Whathecode opened this issue Oct 6, 2024 · 1 comment · Fixed by #487
Labels
enhancement Nice to have, non-functional requirements.
Milestone

Comments

@Whathecode
Copy link
Member

Whathecode commented Oct 6, 2024

When DeviceRegistration.deviceDisplayName was introduced (#350), there was some discussion on whether or not it should be optional. I landed on making it optional, but, for some reason still kept the @Required attribute defined on it, which causes it to always be serialized, even if it is null.

It sounds like null makes a fine default field, which when the name isn't set, shouldn't be serialized.

The fact that it was set to @Required caused some confusion in another PR which introduced a new DeviceRegistration field (#484). If it's not needed, it should probably be removed. Doing so will require a version bump of APIs and adding an application service migration to the services which return this type (for backwards compatibility of older clients). This task should thus be bundled with an API upgrade.

@Whathecode Whathecode added the enhancement Nice to have, non-functional requirements. label Oct 6, 2024
@Whathecode Whathecode added this to the 1.3.0 milestone Oct 6, 2024
@Whathecode
Copy link
Member Author

Whathecode commented Oct 6, 2024

@yuanchen233 I created this new task for our discovery that deviceDisplayName can likely be optional in serialization. Since we are targeting a new API release soonish, it may make sense to change this as part of the next version bump in the API.

It will also be a good exercise to make use of the API migration functionality. You can have a look at DeploymentServiceApiMigrator.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Nice to have, non-functional requirements.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant