Make Ident enum

[omer-shtivi]

Ident now is a struct with a quote style:

pub struct Ident {
    pub value: String,
    pub quote_style: Option<char>,

Now it is a bit risky, because if the char isn't the expected it panics:

       assert!(quote == '\'' || quote == '"' || quote == '`' || quote == '[');

Now it is true that it shouldn't never get to it, because when we tokenize we are only allowing those to be the quote style, but it is still risk for panics.

I suggest to move Ident to be an enum:

pub enum Ident {
    Literal(String),
    SingleQoute(String),
    DoubleQoute(String)
    ...
}

Now the Display will be:

impl fmt::Display for Ident {
    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
        match self {
            Literal(value) =>  write!(f, "{}", value),
            SingleQoute(value) => write!(f, "'{}'", value),
            ...
    }
}

And we can also implement a method get_value:

impl Ident {
    fn get_value(&self) -> String {
       match self {
           Literal(value)  |  SingleQoute(value)  ... => return value.clone()
    }
}

If it is acceptable I will add a PR with the code

[AugustoFKL]

@alamb @andygrove @nickolay is this change acceptable?

I'd love to have that as well and could work on that if would be reviewed. Maybe I can split the structure from the replacement implementation, so you don't have to review a big PR at once.

Otherwise, I'd ask, if possible, to close this :)

[nickolay]

The current design lets users access value without first destructuring the enum, and it allows the dialects to implement whatever quoting style they need without changing the Ident type.

I guess you could replace the Option<char> with a non-exhaustive QuoteStyle enum, but I don't see the value of such a change. (Note that I'm just sharing my perspective as it was implemented this way during my "tenure", the decision should be probably made by @alamb as the current maintainer.)

[alamb]

I agree with @nickolay -- it isn't clear to me what problem this proposal would solve. Can't users do something like

let quote = match ident.quote {
  '\''  |  '"' | '`' | '[' ==> quote
  other => return Err("Unknown quote character: {}, other)
};

Which would safely check for various quote characters and provide an error for unsupported quote styles?

[AugustoFKL]

Actually @alamb I think the main problem is that it's an option, which is a pain to handle. The user would need to unwrap it to be able to match as you showed.

But I agree that having an enum to match that would be a pain. If someone wanted the internal value, would need to have the same code for all variants, which doesn't make much sense

[omer-shtivi]

We can have a function on the enum which provide that internal value.
The risk is when an Ident is added a client can mis-handle one of the variants, which having an enum prevents.
Example:

pub enum Ident {
    Literal(String),
    SingleQoute(String),
    DoubleQoute(String)
    ...
}
impl Ident {
  fn get_value(&self) -> String {
     match self {
        return ... 
    }
   }
}

and it allows for a client to excessive match all quote style and make sure everything is handled properly.

[AugustoFKL]

Hmmmm I think the idea is OK, but TBH I don't have the need for that, and that doesn't relate to ANSII (I'm aiming to expand the support for ANSII as much as possible here)...

@alamb do you think that's reasonable? Ident is quite stable and used all over the code, and accessible by the public API, so the upper projects would need to do a pretty good refactor if that changes.

[alamb]

Actually @alamb I think the main problem is that it's an option, which is a pain to handle. The user would need to unwrap it to be able to match as you showed.

What about something like

let quote = match ident.quote {
  Some('\'')  |  Some('"') | Some('`') | '[' ==> quote,
  None => return Err("No quote specified"),
  other => return Err("Unknown quote character: {}, other)
};

The risk is when an Ident is added a client can mis-handle one of the variants, which having an enum prevents.

yes, I agree this is a potential issue. What about something like the following, which is inline with other parts of the crate:

pub struct Ident {
    pub value: String,
    pub quote_style: QuoteStyle,
}

/// Valid quoting characters
pub enum QuoteStyle {
    /// Quote with `'`
    SingleQuote,
    /// Quote with `"`
    DoubleQuote, 
   ...
}

impl Display for QuoteStyle {
...
}

[alamb]

I worry that changing Ident to be an enum will cause a substantial amount of downstream code churn, even if the downstream crate doesn't care about quoting

[AugustoFKL]

@alamb I think the QuoteStyle enum is quite good, and enough. The display of Ident would be adjusted, and maybe some rebase will be required on downstream projects, but way too less than adding the variants as proposed by @omer-shtivi.

@omer-shtivi is that enough for you?

[omer-shtivi]

Sounds good I will code it soon

…

On Mon, 3 Oct 2022 at 19:20 AugustoFKL ***@***.***> wrote: @alamb <https://github.com/alamb> I think the QuoteStyle enum is quite good, and enough. The display of Ident would be adjusted, and maybe some rebase will be required on downstream projects, but way too less than adding the variants as proposed by @omer-shtivi <https://github.com/omer-shtivi>. @omer-shtivi <https://github.com/omer-shtivi> is that enough for you? — Reply to this email directly, view it on GitHub <#526 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AUI7VIV62T3IUOJPA3EHKCDWBMBUHANCNFSM5ZGEBKOQ> . You are receiving this because you were mentioned.Message ID: ***@***.***>