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

Make enum field setters easier to use #635

Open
coryan opened this issue Jan 7, 2025 · 3 comments
Open

Make enum field setters easier to use #635

coryan opened this issue Jan 7, 2025 · 3 comments
Labels
good first issue This issue is a good place to started contributing to this repository. priority: p3 Desirable enhancement or fix. May not be included in next release. sidekick Issues related to the code generator type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design.

Comments

@coryan
Copy link
Contributor

coryan commented Jan 7, 2025

The setters for enum fields are too hard to use. Consider:

/// Sets the value of `call_log_level`.
pub fn set_call_log_level<T: Into<crate::model::workflow::CallLogLevel>>(
mut self,
v: T,
) -> Self {

That consumes the enum field. We would need to initialize these with:

let workflow = wf::model::Workflow::default()
                .set_call_log_level(
                    wf::model::workflow::CallLogLevel::default()
                        .set_value(wf::model::workflow::call_log_level::LOG_ERRORS_ONLY),
                )

Ugh.... maybe we can implement From<&str> for the enums, so we can write:

let workflow = wf::model::Workflow::default()
                .set_call_log_level(wf::model::workflow::call_log_level::LOG_ERRORS_ONLY))
@coryan coryan added good first issue This issue is a good place to started contributing to this repository. type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design. priority: p3 Desirable enhancement or fix. May not be included in next release. sidekick Issues related to the code generator labels Jan 7, 2025
@fiadliel
Copy link

I think the best thing here (for the user) is that set_call_log_level takes a CallLogLevel value. An Into<CallLogLevel> is possible of course, but here having a From<&str> feels very easy to mix up with an incorrect enum (for the user).

So then, what about if the enums generated by the code are not &str, but actually are values of CallLogLevel?

Two approaches here:

  1. The value inside CallLogLevel could be a Cow<'static, str>, which lets you generate const values for each enum, these can actually return a CallLogLevel value.
  2. Use an actual rust enum (a more usual approach for rust code)? But I see you already decided against that in impl(generator): Rust uses strings for enum values #88

So to be more concrete:

pub struct CallLogLevel(std::borrow::Cow<'static, str>);

impl CallLogLevel {
    pub const fn new(v: &'static str) -> Self {
        Self(std::borrow::Cow::Borrowed(v))
    }
}

pub mod call_log_level {
  pub const LOG_ERRORS_ONLY: CallLogLevel = CallLogLevel::new("LOG_ERRORS_ONLY");
}

@coryan
Copy link
Contributor Author

coryan commented Jan 10, 2025

So then, what about if the enums generated by the code are not &str, but actually are values of CallLogLevel?

I like that idea. Safer and we can still support unexpected enum values.

Use an actual rust enum (a more usual approach for rust code)? But I see you already decided against that in #88

Yes. We did not provide a full rationale: the client must be prepared to receive unexpected values for enums. Services may add new values without notice. And it is nice if the clients can send new values without having to upgrade the library.

@fiadliel
Copy link

Yes. We did not provide a full rationale: the client must be prepared to receive unexpected values for enums. Services may add new values without notice. And it is nice if the clients can send new values without having to upgrade the library.

Obviously a necessity. The main sad thing is the loss of robust pattern matching when handling the service response.

I won't belabor the point, but did you consider a wrapper type to handle the "unknown value" issue?

e.g.

pub enum MaybeEnum<A> {
    /// A known value
    Known(A),
    /// An unknown value that can be handled in more limited ways
    Unknown(String),
}

impl<A> MaybeEnum<A> {
    #[must_use]
    pub fn new(s: &str) -> MaybeEnum<A>
    where
        A: FromStr,
    {
        s.parse()
            .map_or_else(|_| Self::Unknown(s.to_string()), |v| Self::Known(v))
    }
}

impl<A> From<A> for MaybeEnum<A> {
    fn from(value: A) -> Self {
        Self::Known(value)
    }
}

This would let you either match on the internal A, or see that you got a value not currently known by the API client you're using.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue This issue is a good place to started contributing to this repository. priority: p3 Desirable enhancement or fix. May not be included in next release. sidekick Issues related to the code generator type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design.
Projects
None yet
Development

No branches or pull requests

2 participants