search

novifinancial / serde-reflection

Rust libraries and tools to help with interoperability and testing of serialization formats based on Serde.
Apache License 2.0
139 stars 42 forks source link

Add support for Dart 2.14+ #112

Closed jerel closed 3 years ago

jerel commented 3 years ago

Summary

This PR brings #57 up to date with the latest code on main and also adds a number of fixes and improvements. As we have a project based on serde-reflection we have motivation to keep the Dart implementation current.

Generated Dart code follows the official Effective Dart guide and CI requires generated code to pass dart analyze as well as dart test.

Test Plan

The following usage should produce a working Dart project:

let registry = test_utils::get_registry().unwrap();

let config = CodeGeneratorConfig::new(String::from("example"))
    .with_encodings(vec![Encoding::Bincode])
    .with_c_style_enums(true);

let package = PathBuf::from("dart_example");
let installer = dart::Installer::new(package.clone());
installer.install_bincode_runtime().unwrap();
installer.install_bcs_runtime().unwrap();
installer.install_serde_runtime().unwrap();
installer.install_module(&config, &registry).unwrap();
ma2bd commented 3 years ago

Impressive, let me have a look!

ma2bd commented 3 years ago

Not sure why we don't see the result of the CI (which we definitely need). Perhaps try to rebase?

jerel commented 3 years ago

@matbd thanks for the feedback, I believe I've addressed all points.

jerel commented 3 years ago

Not sure why we don't see the result of the CI (which we definitely need). Perhaps try to rebase?

For future reference, I had to sign in to CircleCI for the CI to run. Apparently for PRs it uses the pull requester's account and not the organization's.

jerel commented 3 years ago

@matbd is anything further needed before this can be merged and released? I'm ready on my side.

ma2bd commented 3 years ago

Alright, I just managed to test things locally, and will make a release shortly. I'm also going to fix some clippy warnings that CI did not report for some reasons.

ma2bd commented 3 years ago

@jerel It seems that I missed a few issues and landed the PR a bit too early. Would you be able to send me a follow-up PR to add at least a simple runtime test like the one in typescript? https://github.com/novifinancial/serde-reflection/blob/main/serde-generate/tests/typescript_runtime.rs

ma2bd commented 3 years ago

Also it looks like the CI is not quite stable yet?

https://app.circleci.com/pipelines/github/novifinancial/serde-reflection/708/workflows/cb11f734-e46b-48bd-a315-94bf4564729f/jobs/2106

jerel commented 3 years ago

@matbd yep, I'll take a look

ma2bd commented 3 years ago

Much appreciated. Thanks