There are currently no standardized naming conventions for constructor methods. For example, consider these methods:
QDate::from_string_enum(string: &QString, format: DateFormat) -> Option<Self>
QDateTime::from_string(string: &QString, format: DateFormat) -> Option<Self>
QTime::from_string_enum(string: &QString, format: DateFormat) -> SelfQDate::from_string_enum and QDateTime::from_string have the same signature, but different names.
QDate::from_string_enum and QTime::from_string_enum have the same name, but different signatures.
Proposal 1: Constructor methods should be the names of each parameter type, prefixed by from_. If they return Option<Self> rather than Self, the name should be suffixed by _opt, which is a common pattern in other crates. So the above would be replaced by:
QDate::from_qstring_dateformat_opt(string: &QString, format: DateFormat) -> Option<Self>
QDateTime::from_qstring_dateformat_opt(string: &QString, format: DateFormat) -> Option<Self>
QTime::from_qstring_dateformat(string: &QString, format: DateFormat) -> SelfProposal 2: Deprecate all the constructor methods and just use From and TryFrom with tuples. So the above would be replaced by:
impl TryFrom<(&QString, DateFormat)> for QDate
impl TryFrom<(&QString, DateFormat)> for QDateTime
impl From<(&QString, DateFormat)> for QTimeProposal 3: Do proposal 1 and the implementations from proposal 2.
Activity
4
ahayzen-kdab1y agoI think the same, proposal 1 and then use _or_default instead of _opt.
And as noted in proposal 3, we can always add proposal 2 later if we think it could be useful and do both.
jnbooth1y ago@ahayzen-kdab I would expect _opt to mean "returns None if invalid", and _or_default to mean "returns Default::default() if invalid". So in this case, QTime::from_qstring_or_default would return the parsed time if valid, or the current time if not, which would be somewhat confusing.
LeonMatthesKDAB1y agoHm, it's true that the Default::default implementations don't always match.
I think for the existing methods where we used the _or_default suffix it should be the case, as for many types we have implemented Default::default() to return an empty/null type.
But in places where this isn't the case, we could use an _or_null/_or_invalid suffix instead of _or_default.
My vote is for Proposal 1, that follows the conventions I'm most used to from other Rust crates.
A note on the
_optsuffix though: Qt often returns "null types" for invalid conversions, for example forQTime. At the moment we (mostly, but not consistently) wrap these methods such that they returnOption<Self>and provide a variant suffixed with_or_defaultthat directly calls the Qt function. This is basically the opposite of the_optsuffix.I prefer to keep the
_or_defaultsuffix and return Option for the non-suffix function, as that would encourage checking the Option, which is more Rust idiomatic. With an_optsuffix, we'd encourage users to call the non-_optfunction, which can return invalid/null results.@ahayzen-kdab What's your opinion on this?