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: Support for marking "RUST-ATTRIBUTE" (type attribute) #405

Merged
merged 3 commits into from
Jul 27, 2024

Conversation

H2Sxxa
Copy link
Contributor

@H2Sxxa H2Sxxa commented Jul 21, 2024

Changes

Support pass extra args to protoc.

Why

Sometimes, there is a need to add derives (#401) , if we use protoc to generate, we can pass some optional arguments,
like --prost_opt=type_attribute=Helloworld=#[derive(Foo)], but in rinf, we can't do the same.

This PR allows you to pass extra args to protoc and you can configure the extra args in pubspec.yaml.

Usage

  1. Configure in pubspec.yaml.
rinf:
  message:
    extra_args: 
      - --prost_opt=type_attribute=SampleFractal=#[derive(Foo)]
  1. Run rinf message
fractal_art.rs

// @generated
/// \[RINF:RUST-SIGNAL-BINARY\]
/// You can add your custom comments like this.
/// Protobuf's import statement also works well.
+ #[derive(Foo)]
#[allow(clippy::derive_partial_eq_without_eq)]
#[derive(Clone, PartialEq, ::prost::Message)]
pub struct SampleFractal {
...

FYI, https://github.com/neoeinstein/protoc-gen-prost/tree/main/protoc-gen-prost#options

Before Committing

dart analyze flutter_ffi_plugin --fatal-infos
dart format .
cargo fmt
cargo clippy --fix --allow-dirty

@temeddix
Copy link
Member

temeddix commented Jul 21, 2024

Great PR :)

I have one request. Rather than passing raw extra_args, could you make the config format include rust_derive field as written below?

rinf:
  message:
    rust_derive: 
      SampleFractal: "Default"
      NumberInput:
        - "Default"
        - "extern_crate::ExternTrait"

@H2Sxxa
Copy link
Contributor Author

H2Sxxa commented Jul 21, 2024

Great PR :)

I have one request. Rather than passing raw extra_args, could you make the config format include rust_derive field as written below?

rinf:
  message:
    rust_derive: 
      SampleFractal: "Default"
      NumberInput:
        - "Default"
        - "extern_crate::ExternTrait"

I agree, the request is reasonable.

But the type_attribute not only includes derives, there are some stuff like #[serde(rename_all = "snake_case")] .

How about keeping both options at the same time?

rinf:
  message:
    rust_derive: 
      SampleFractal: "Default"
      NumberInput:
        - "Default"
        - "extern_crate::ExternTrait"
    type_attribute:
      SampleFractal: "#[derive(Trait)]"
      NumberInput:
        - "#[serde(rename_all = \"snake_case\")]"
        - ...

@temeddix
Copy link
Member

There's actually another option, which is the most ideal way in my opinion.

It's allowing to write something like:

// [RINF:DART-SIGNAL]
// [RINF:RUST-ATTRIBUTE(#[serde(rename_all = "camelCase")])]
message MyMessage {}

What do you think? Do you mind orienting your PR this way?

@H2Sxxa
Copy link
Contributor Author

H2Sxxa commented Jul 21, 2024

There's actually another option, which is the most ideal way in my opinion.

It's allowing to write something like:

// [RINF:DART-SIGNAL]
// [RINF:RUST-ATTRIBUTE(#[serde(rename_all = "camelCase")])]
message MyMessage {}

What do you think? Do you mind orienting your PR this way?

That's great.

@H2Sxxa
Copy link
Contributor Author

H2Sxxa commented Jul 21, 2024

I tried to implement it using a method that was less logically disruptive to the code.

If you want to implement it yourself, just feel free to close the PR or refactor it :)

@H2Sxxa H2Sxxa changed the title Support pass extra args to protoc feat: Support for marking "RUST-ATTRIBUTE" (type attribute) Jul 21, 2024
@H2Sxxa H2Sxxa marked this pull request as draft July 21, 2024 06:11
@H2Sxxa H2Sxxa marked this pull request as ready for review July 21, 2024 06:18
@temeddix
Copy link
Member

Looks good overall :D I will add documentations and refine some of the code later. Thank you for your effort and patience.

@temeddix temeddix merged commit 076feb6 into cunarist:main Jul 27, 2024
22 checks passed
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