[pve-devel] [PATCH proxmox v3 2/2] network-types: add hostname type

[Wolfgang Bumiller]

On Fri, Apr 04, 2025 at 01:26:46PM +0200, Stefan Hanreich wrote:
> 
> 
> On 4/4/25 09:51, Stefan Hanreich wrote:
> >>> +///
> >>> +/// It checks for the following conditions:
> >>> +/// * At most 63 characters long.
> >>> +/// * It must not start or end with a hyphen.
> >>> +/// * Must only contain ASCII alphanumeric characters as well as hyphens.
> >>> +/// * It must not be purely numerical.
> >>> +#[derive(Debug, Deserialize, Serialize, Clone, Eq, Hash, PartialOrd, Ord, PartialEq)]
> >>> +pub struct Hostname(String);
> >>> +
> >>> +impl std::str::FromStr for Hostname {
> >>> +    type Err = HostnameError;
> >>> +
> >>> +    fn from_str(hostname: &str) -> Result<Self, Self::Err> {
> >>> +        Self::new(hostname)
> >>> +    }
> >>> +}
> >>> +
> >>> +impl AsRef<str> for Hostname {
> >>> +    fn as_ref(&self) -> &str {
> >>> +        &self.0
> >>> +    }
> >>> +}
> >>> +
> >>> +impl Display for Hostname {
> >>> +    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
> >>> +        self.0.fmt(f)
> >>> +    }
> >>> +}
> >>> +
> >>> +impl Hostname {
> >>> +    /// Constructs a new hostname from a string
> >>> +    ///
> >>> +    /// This function accepts characters in any case, but the resulting hostname will be
> >>> +    /// lowercased.
> >>> +    pub fn new(name_ref: impl AsRef<str>) -> Result<Self, HostnameError> {
> >>
> >> Nit: I'd recommend using a `check()` function which does not create the
> >> `Hostname` itself, because then:
> >>
> >> - in `FromStr` we know we have a reference (&str) and need to clone.
> >> - We could add a `TryFrom<&str>` wich just uses `.parse()`
> >> - We could add a `TryFrom<String>` which avoids the clone.
> 
> What about a constructor that just takes String (if a function needs to
> own the value it should demand a String anyway) and then calling that
> constructor from the proposed trait implementations? Maybe something
> more generic than String as a param, but that could usually be tacked on
> later without breaking the API.

I suppose that's fine. We'd clone in the error case, but that's not the
expected case anyway.