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

Upgrade syn, Migrate SerializeHierarchy to PathSerialize, PathDeserialize, PathIntrospect #987

Merged
merged 11 commits into from
May 5, 2024

Conversation

schmidma
Copy link
Member

@schmidma schmidma commented May 1, 2024

Introduced Changes

This PR upgrades the syn crate to the newest release, which included several breaking changes.

While porting our derive macros (approx_derive and serialize_hierarchy_derive), which heavily depend on syn, I came up with a newer, cleaner way to express the functionality of our path serialization -> Now called path_serde.

Instead of a single trait for all the necessary functionality, I define 3 new traits.

pub trait PathSerialize {
    fn serialize_path<S>(&self, path: &str, serializer: S) -> Result<S::Ok, Error<S::Error>>
    where
        S: Serializer;
}

pub trait PathDeserialize {
    fn deserialize_path<'de, D>(
        &mut self,
        path: &str,
        deserializer: D,
    ) -> Result<(), Error<D::Error>>
    where
        D: Deserializer<'de>;
}

pub trait PathIntrospect {
    fn get_fields() -> BTreeSet<String> {
        let mut fields = BTreeSet::default();
        Self::extend_with_fields(&mut fields, "");
        fields
    }

    fn extend_with_fields(fields: &mut BTreeSet<String>, prefix: &str);
}

All usages are subsequently adjusted to fit the new separation. Trait bounds can be more specific, and the exists(path: String) function was removed as it is duplication of a get_fields().contains(path) call.

ToDo / Known Issues

  • documentation

Ideas for Next Iterations (Not This PR)

  • clean up implementation of communication
  • guess the bounds for generics in derive of path_serde traits
  • non-leaf additional fields. This allows something like VisionTop.main.image.rbg.jpeg. Currently the conversion to rgb is implicitly done when converting to jpeg

How to Test

hopefully only compilation...
But better test the communication server with twix or fanta etc...

@schmidma schmidma enabled auto-merge May 2, 2024 12:44
@knoellle knoellle self-assigned this May 3, 2024
This was referenced May 3, 2024
crates/path_serde/src/deserialize.rs Outdated Show resolved Hide resolved
crates/path_serde_derive/src/container.rs Outdated Show resolved Hide resolved
crates/path_serde_derive/src/container.rs Outdated Show resolved Hide resolved
docs/framework/path_serde.md Outdated Show resolved Hide resolved
docs/framework/path_serde.md Outdated Show resolved Hide resolved
docs/setup/configure_team.md Outdated Show resolved Hide resolved
docs/setup/development_environment.md Outdated Show resolved Hide resolved
docs/setup/development_environment.md Outdated Show resolved Hide resolved
docs/setup/development_environment.md Outdated Show resolved Hide resolved
docs/setup/upload.md Outdated Show resolved Hide resolved
docs/setup/upload.md Outdated Show resolved Hide resolved
docs/setup/upload.md Outdated Show resolved Hide resolved
Copy link
Contributor

@knoellle knoellle left a comment

Choose a reason for hiding this comment

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

😅

docs/setup/nao_setup.md Outdated Show resolved Hide resolved
docs/setup/nao_setup.md Outdated Show resolved Hide resolved
docs/setup/nao_image_and_sdk.md Outdated Show resolved Hide resolved
docs/setup/nao_image_and_sdk.md Outdated Show resolved Hide resolved
docs/setup/nao_image_and_sdk.md Outdated Show resolved Hide resolved
docs/setup/nao_image_and_sdk.md Outdated Show resolved Hide resolved
docs/setup/nao_image_and_sdk.md Outdated Show resolved Hide resolved
docs/setup/nao_image_and_sdk.md Outdated Show resolved Hide resolved
docs/setup/nao_image_and_sdk.md Outdated Show resolved Hide resolved
docs/setup/nao_image_and_sdk.md Outdated Show resolved Hide resolved
Copy link
Contributor

@knoellle knoellle left a comment

Choose a reason for hiding this comment

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

LGTM

@schmidma schmidma added this pull request to the merge queue May 5, 2024
Merged via the queue into HULKs:main with commit 252fe4b May 5, 2024
22 checks passed
@schmidma schmidma deleted the upgrade-syn branch May 5, 2024 12:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants