Streamline quoting
I was very suprised and confused when I noticed the following in the codebase:
- A comment on
Iden::quoted, saying that it doesn't actually return a quoted string (!). In fact, the quoting is done later by the (DB-specific) backend. -
fn is_static_identhat's not actually about knowing the identifier at compile time. In fact, it's about having special characters in the identifier (and the need to escape the identifier in the backend). - The expensive use of
Cow::Ownedto indicate the presense of special characters (rather than to simply store a runtime-provided string).
It's so weird and non-obvious.
Here, I streamline things:
-
Removed the
Iden::quotedmethod that isn't actually quoted.I understand that this is a breaking change. But if the users can't actually get a properly-quoted string without a specific DB backend, then we should help them notice this reality.
-
Made
Iden::unquotedreturn aCow, as that's the method that actually constructs aDynIden(Cow).This is a breaking change.
-
Replaced
IdenStatic: Idenwithimpl<T> Iden for T where T: IdenStatic.Technically a breaking change for manual implementors, but most people probably use derives.
-
Removed all other extra supertraits from
IdenStatic. All tests pass without those, and it's not clear why those were needed. Those are probably leftowers from the oldIdensystem. If those are actually needed, we should test and document that.Technically a breaking change, although I doubt that many people worked with generic
IdenStaticand relied on those implied bounds. -
Replaced
impl Idenwithimpl IdenStaticeverywhere where the string is static (e.g., in all derives). -
Removed unnecessary allocations of
Cow::Owned. -
Removed
is_static_iden.This isn't a breaking change because we didn't have that in
1.1.x. -
Updated the docs.
To be fair, I understand the intended meaning and benefit of is_static_iden and indicating it with Cow::Borrowed. When we know at compile time that the identifier doesn't have any special characters, the backend can write_str it as-is, in one go. Because that's no longer the case, my PR may regress the performance a little.
However:
- The correctness of that approach is hard to verify. I'm not sure that we construct
Cow::Borrowedonly when the string doesn't have special characters. It's easy to chooseCow::Borrowedsimply because you have a static string (whether or not it contains special characters). That's the most obvious thing to do. - My PR wins back performance by avoiding allocations and using
Cow::Borrowedmore often. - If we really want to keep a fast
write_strpath in the backend, we can put theis_static_idencheck right there before the write. But I wouldn't worry about that.
The main thing that I'm going for here is conceptual clarity, rather than performance:
- All static identifiers now implement
IdenStatic. - No more
qoutedthat isn't actually quoted. - No more valid static identifiers that return
falsefromis_static_iden.