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: Use serde_repr to (de)serialize Mode and Status enums. #15

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

dlips
Copy link

@dlips dlips commented Dec 21, 2023

This pull request changes the manual deserialization implementation for the Mode enum to use the serde_repr crate. Due to this change, the behavior on deserialization is modified. Before all unknown values where mapped to Mode::NoFix. Now only the 0 is mapped to Mode::Unknown as defined in the gpsd json protocol. Non-defined values result in a serialization error.

The status field is now also supported as Status enum. Implementation aligns with the new implementation for the Mode enum.

The changes would required increasing the major version.

Initial motiviation for the changes was that the serialize feature was not working due to the fact that there was not Serialize implementation for the Mode enum.

@dlips dlips force-pushed the feature/improved_enum_supoort branch from 9f47e67 to 85ade7e Compare December 21, 2023 13:20
@dlips
Copy link
Author

dlips commented Dec 22, 2023

I just realized that the serialization is not fully working as expected because the class field is missing in the generated json.

@bwolf
Copy link
Owner

bwolf commented Jan 12, 2024

Kindly want to ask: what Do you want to achieve with this?

@bwolf
Copy link
Owner

bwolf commented Jan 12, 2024

How should we continue with this? Can you write some tests which fail for your use cases? I think that could be a start. From there we could try to find a route.

@dlips
Copy link
Author

dlips commented Jan 29, 2024

Kindly want to ask: what Do you want to achieve with this?

The goal of replacing the numerical values with enums is to make it more typesafe and improve code readability when matching the values of Status and Mode. Alternative would be to define constants for each numerical value.

How should we continue with this? Can you write some tests which fail for your use cases? I think that could be a start. From there we could try to find a route.

I wanted to use the serialization functionality in my tests where I created a fake gpsd server to be used in my integration test. Currently I am using hardcoded json string, which is not optimal for future extensions of test cases.

I will add test cases as you suggested.

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