Skip to content

Add serde behind feature flag for some builtins #237

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

Merged
merged 1 commit into from
May 8, 2023

Conversation

jrockett6
Copy link
Contributor

Pretty straightforward here, though I hope I got all the types that made sense. I'm not familiar with the FFI stuff, so to be safe I left out anything with "Opaque" fields. Let me know if there is anything else here that I can help with.

@Bromeon Bromeon added feature Adds functionality to the library c: core Core components labels Apr 19, 2023
Copy link
Member

@Bromeon Bromeon left a comment

Choose a reason for hiding this comment

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

Thanks for this!

In godot-rust/gdnative#743, we decided against serializing Rid because it's not a persistent value.

Also, implementing serde support entails more than the derive-macros, namely:

  1. Finding a persisted representation for each type -- this is not always a 1:1 mapping of the struct/enum definition. For example for Projection, should we just store cols or something else?
  2. This would need to be accompanied by tests, serde_json should be enough. Tests also serve as expressive code that visualizes how the persisted format looks.
  3. While this wouldn't need to be implemented right now, it's probably good to think about how to support composite types like Array, PackedArray, Dictionary, Variant.

In general, it may be a bit early to decide all the things already. Some of the interfaces are still changing (including composite types). Also, we'd need to check that this doesn't make our CI run significantly longer; or just enable serde in the Linux unit-test.

@Bromeon
Copy link
Member

Bromeon commented Apr 30, 2023

@jrockett6 For an initial version, we would need concretely:

  • Removal of support for Rid
  • 1 test per type (based on serde_json). Each such test could reuse a helper function like this:
    fn roundtrip<T>(value: &T, expected_json: &str)
        where T: Deserialize + Serialize + PartialEq
    {
        // pseudo-code
        let json: String = serialize(value);
        let back: T = deserialize(json);
    
        assert_eq!(back, value, "serde round-trip changes value");
        assert_eq!(json, expected_value, "value does not conform to expected JSON");
    }
  • enable the feature in the CI unit-test, but only for 1 job (the Linux one)

Do you think that's possible?

@jrockett6
Copy link
Contributor Author

Yep, sounds good. I'll get a revision out this week.

@jrockett6 jrockett6 marked this pull request as draft May 4, 2023 05:02
@jrockett6 jrockett6 force-pushed the serde_for_builtin_types branch 2 times, most recently from 25d51d3 to ced187f Compare May 4, 2023 19:01
@jrockett6
Copy link
Contributor Author

jrockett6 commented May 4, 2023

@Bromeon I updated to include tests and remove RID support, cargo test -p godot-core --features serde runs successfully.

I think the only outstanding thing for the initial version (apart from potential review changes) is adding the feature flag to the linux CI. I'm a bit lost on where this should be added though, some suggestions on how to modify the yaml file would be very helpful 😁

@Bromeon
Copy link
Member

Bromeon commented May 8, 2023

Sure! 🙂
See e.g. this step in the CI workflow.

You can extend/rename:

          - name: linux-threads
            os: ubuntu-20.04
            artifact-name: linux
            godot-binary: godot.linuxbsd.editor.dev.x86_64
            rust-extra-args: --features threads

as follows:

          - name: linux-features
            os: ubuntu-20.04
            artifact-name: linux
            godot-binary: godot.linuxbsd.editor.dev.x86_64
            rust-extra-args: --features threads,serde

@jrockett6 jrockett6 force-pushed the serde_for_builtin_types branch from ced187f to ad52498 Compare May 8, 2023 20:32
@GodotRust
Copy link

API docs are being generated and will be shortly available at: https://godot-rust.github.io/docs/gdext/pr-237

@Bromeon
Copy link
Member

Bromeon commented May 8, 2023

Thank you! 🙂

bors r+

@Bromeon Bromeon marked this pull request as ready for review May 8, 2023 20:50
bors bot added a commit that referenced this pull request May 8, 2023
237: Add serde behind feature flag for some builtins r=Bromeon a=jrockett6

Pretty straightforward here, though I hope I got all the types that made sense. I'm not familiar with the FFI stuff, so to be safe I left out anything with "Opaque" fields. Let me know if there is anything else here that I can help with.

Co-authored-by: jrockett6 <[email protected]>
@bors
Copy link
Contributor

bors bot commented May 8, 2023

Build failed:

@jrockett6 jrockett6 force-pushed the serde_for_builtin_types branch from ad52498 to bf6fea7 Compare May 8, 2023 21:02
@Bromeon
Copy link
Member

Bromeon commented May 8, 2023

bors r+

@bors
Copy link
Contributor

bors bot commented May 8, 2023

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

  • full-ci

@bors bors bot merged commit c0935ac into godot-rust:master May 8, 2023
@jrockett6 jrockett6 deleted the serde_for_builtin_types branch May 8, 2023 21:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: core Core components feature Adds functionality to the library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants