prost icon indicating copy to clipboard operation
prost copied to clipboard

Fine-grained selection of where to add type or field attributes using filters

Open marjakm opened this issue 2 years ago • 9 comments

This does not break the external API, it only adds 2 new functions


pub fn type_attribute_with_filter<P, A, F>(
    &mut self,
    path: P,
    attribute: A,
    filter: F,
) -> &mut Self;
where
    P: AsRef<str>,
    A: AsRef<str>,
    F: Into<TypeFilter>
    
pub fn field_attribute_with_filter<P, A, TF, LF, FF>(
    &mut self,
    path: P,
    attribute: A,
    type_filter: TF,
    label_filter: LF,
    field_filter: FF,
) -> &mut Self;
where
    P: AsRef<str>,
    A: AsRef<str>,
    TF: Into<TypeFilter>,
    LF: Into<LabelFilter>,
    FF: Into<FieldFilter>

and 6 types - 3 pairs of Filter and Selector:

TypeFilter, TypeSelector,
LabelFilter, LabelSelector, 
FieldFilter, FieldSelector, 

Filters are bitsets (newtypes around u32) and Selectors give meaning to values in that bitset (c-like enums with selected discriminator values). Attribute maps contain these filters and attributes are only applied if all filters match.

    type_attributes: PathMap<(String, TypeFilter)>,
    field_attributes: PathMap<(String, TypeFilter, LabelFilter, FieldFilter)>,

This allows very specific selection of where to add attributes, example:

        .type_attribute(".", "#[derive(serde::Serialize, serde::Deserialize)]")
        // enums with data should use serde adjacently tagged format
        .type_attribute_with_filter(
            ".",
            "#[serde(tag = \"kind\", content = \"data\")]",
            TypeSelector::RustEnumWithData
        )
        // All c-like enums should have clap::ArgEnum impl
        .type_attribute_with_filter(
            ".",
            "#[derive(clap::ArgEnum)]",
            TypeSelector::RustEnumCLike
        )
        // and structs a clap::Parser impl
        .type_attribute_with_filter(
            ".",
            "#[derive(clap::Parser)]",
            TypeSelector::RustStruct
        )
        // use a trait to find parsers for non-scalar types
        .field_attribute_with_filter(
            ".",
            "#[clap(parse(try_from_str=crate::CustomClapParse::parse))]",
            TypeSelector::RustStruct,
            LabelSelector::Everything,
            !FieldSelector::ProtobufScalar | FieldSelector::Bytes,
        )
        // repeated String also needs the custom parser
        .field_attribute_with_filter(
            ".",
            "#[clap(parse(try_from_str=crate::CustomClapParse::parse))]",
            TypeSelector::RustStruct,
            LabelSelector::Repeated,
            FieldSelector::String
        )

This probably needs some tests, but that would be quite pointless if you don't like this feature - if this seems reasonable, I can add a bunch of tests.

Previous pull request had a similar idea, but implemented it differently: https://github.com/tokio-rs/prost/pull/494

May solve cases like these: https://github.com/tokio-rs/prost/issues/371 https://github.com/tokio-rs/prost/issues/332

marjakm avatar Oct 19 '21 19:10 marjakm

@marjakm this is a great improvement, we've been suffering the lack of this fine-grained functionality for a long time!

@danburkert this would nicely solve our problem in #371 .

cc @Veetaha

anelson avatar Nov 04 '21 15:11 anelson

I don't quite understand why the github actions failed, doesn't seem like related to anything I changed:

 error: failed to parse manifest at `/home/runner/.cargo/registry/src/github.com-1ecc6299db9ec823/half-1.8.1/Cargo.toml`
Error: failed to parse manifest at `/home/runner/.cargo/registry/src/github.com-1ecc6299db9ec823/half-1.8.1/Cargo.toml`
Caused by:
  feature `resolver` is required

  this Cargo does not support nightly features, but if you
  switch to nightly channel you can add
  `cargo-features = ["resolver"]` to enable this feature
Error: The process '/usr/share/rust/.cargo/bin/cargo' failed with exit code 101

marjakm avatar Nov 04 '21 16:11 marjakm

@LucioFranco any comments?

marjakm avatar Dec 09 '21 15:12 marjakm

any progress?

sunqi1993 avatar Jan 10 '22 02:01 sunqi1993

@marjakm you will need to rebase against master to get CI working again

LucioFranco avatar Jan 21 '22 18:01 LucioFranco

I'll be on vacation for a week starting tomorrow, I'll do it afterwards (just to let you know, I'm still interested in getting this merged).

marjakm avatar Jan 21 '22 19:01 marjakm

This feature would be really handy for adding custom derivations to particular types i.e. strum's variety of derive functions for enums.

Can I enquire if there's any progress?

XL-Lewis avatar Aug 22 '22 04:08 XL-Lewis

I think something like this could eventually be merged but I am not sure atm if this is the right approach.

LucioFranco avatar Aug 22 '22 15:08 LucioFranco

Squashed commits and rebased onto master

marjakm avatar Sep 06 '22 20:09 marjakm

any updates on this? running into many of the same issues, re: specificity with type/field attributes applied to nested defs

sunny-g avatar Dec 20 '22 00:12 sunny-g