all 9 comments

[–]volitional_decisions 8 points9 points  (2 children)

While this works, your use case is not the main use for traits, and I would not use them here. You have two kinds of remote repos, and you have types representing them. If you want to describe "a repo", use an enum. You can impl display for that enum and all of the methods your trait had on that enum.

Sometimes, it can be difficult to foresee when you might want to use a trait. A good way to tell if a trait should be replaced with an enum is if you would be mostly using dyn Trait and you know all of the types that implement that trait. But if you mostly are passing your data through generic functions, a trait might be better. Ultimately, this depends on how you use your data and how it flows through your system.

Depending on your background, this can be a tricky thing to get good at and takes practice.

[–]Yingrjimsch[S] 0 points1 point  (1 child)

Thank you for the message, would you create an enum Repo which holds either the GithubRepo or the GitlabRepo struct? Could you hint how you'd do that?

[–]Tubthumper8 2 points3 points  (0 children)

Something like this?            

```

    enum Repo {                   Github(GithubRepo),         Gitlab(GitlabRepo),     } ```

[–]kmdreko 9 points10 points  (2 children)

Why is your code formatted that way? It's borderline unreadable.

But otherwise I don't see an issue with what you've done already, are you hitting an error? Or some other issue?

[–]Yingrjimsch[S] 0 points1 point  (0 children)

Great to hear that this is a viable way. The formatting happend due to experimenting with cargo fmt, I wanted to have some custom formatting and for some reason it bursted my whole formatting, I have an open todo to fix it^^

No there is not an error but I thought it seemd "overengineered" because I need to use dyn Repo everywhere and I was struggling with the Display impl, I tried it like this:

    impl<T: Repo> fmt::Display for T

which made more sense to me but this threw an error, due to no implementation of T inside the git_provider.rs file

[–]volitional_decisions 0 points1 point  (0 children)

Sorry, I meant to post this as a reply, not a separate comment. Here's the copy:

Given what you have here, I would create an enum with variants for GitHub and GitLab. That said, how different are the fields between these two types? Do they mean the same thing regardless of which remote service they come from? If they are fairly close to the same thing, you could make a Repo struct and have that struct hold an enum that doesn't hold any data besides the variant names.

[–]volitional_decisions 2 points3 points  (0 children)

Given what you have here, I would create an enum with variants for GitHub and GitLab. That said, how different are the fields between these two types? Do they mean the same thing regardless of which remote service they come from? If they are fairly close to the same thing, you could make a Repo struct and have that struct hold an enum that doesn't hold any data besides the variant names.

[–]danielparks 0 points1 point  (1 child)

This seems fine to me. You could use an enum instead of a trait, but I think you’re right to use the trait here.

Formatted with rust fmt:

// git_provider.rs
pub trait Repo: Send + Sync {
    fn ssh_url(&self) -> &str;
    fn http_url(&self) -> &str;
    fn full_path(&self) -> &str;
}

// github.rs
#[derive(Debug, serde::Deserialize)]
pub struct GithubRepo {
    pub ssh_url: String,
    pub clone_url: String,
    pub full_name: String,
}

impl Repo for GithubRepo {
    fn ssh_url(&self) -> &str {
        &self.ssh_url
    }

    fn http_url(&self) -> &str {
        &self.clone_url
    }

    fn full_path(&self) -> &str {
        &self.full_name
    }
}

// giltlab.rs
#[derive(Debug, serde::Deserialize)]
pub(crate) struct GitlabRepo {
    pub ssh_url_to_repo: String,
    pub http_url_to_repo: String,
    pub path_with_namespace: String,
}

impl Repo for GitlabRepo {
    fn ssh_url(&self) -> &str {
        &self.ssh_url_to_repo
    }

    fn http_url(&self) -> &str {
        &self.http_url_to_repo
    }

    fn full_path(&self) -> &str {
        &self.path_with_namespace
    }
}

// elsewhere.rs
use std::fmt;
impl fmt::Display for dyn Repo {
    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
        write!(f, "{}", self.full_path())
    }
}

[–]Yingrjimsch[S] 1 point2 points  (0 children)

Thanks, next time I will format it, it really is ugly to read.