RazrFalcon / roxmltree

Represent an XML document as a read-only tree.
Apache License 2.0
434 stars 37 forks source link

Avoid redundant tuple layer for ExpandedName::from_static #101

Closed adamreichold closed 1 year ago

adamreichold commented 1 year ago

Since the method has to be called explicitly, the tuple layer serves no functional purposes.

This split in API conventions has precedence in the standard library, e.g. HashMap::retain accepting a two-argument closure versus impl FromIterator for HashMap expecting key-value pairs.

cc @WhyNotHugo

RazrFalcon commented 1 year ago

Thanks!

WhyNotHugo commented 1 year ago

Makes perfect sense :+1:

WhyNotHugo commented 1 year ago

FWIW, the first argument could be Option<&str> to allow creating ExpandeNames with no namespace too.

Want me to send a patch for that?

adamreichold commented 1 year ago

FWIW, the first argument could be Option<&str> to allow creating ExpandeNames with no namespace too.

Want me to send a patch for that?

https://github.com/RazrFalcon/roxmltree/pull/100#discussion_r1246946216

RazrFalcon commented 1 year ago

@WhyNotHugo For consistency, I guess we can add Option<&str> too.

WhyNotHugo commented 1 year ago

Another possible option is:

    pub const fn from_static_without_namspace(name: &'static str) -> Self {
        Self {
            uri: None,
            name,
        }
    }

The naming is horrible, but I can't think of anything better.

FWIW: I only need the existing variant; I'm only suggesting the Option variant for completeness' sake. But it's also perfectly fair to omit it until somebody actually needs it.

RazrFalcon commented 1 year ago

Yes, the name is too long. Too bad Rust doesn't have named arguments like Swift.

I would suggest adding Option anyway, otherwise the current constructor is partial.