Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(config): Implement std::default::Default #19

Merged
merged 1 commit into from
Jun 6, 2024

Conversation

BKDaugherty
Copy link
Contributor

@BKDaugherty BKDaugherty commented Jun 5, 2024

Hey! I'm hoping to use this library for a project of mine, and wanted to instantiate a Config using the Default trait. Thought I'd put up a PR for it! Lmk what you think!

src/config.rs Outdated Show resolved Hide resolved
@BKDaugherty BKDaugherty force-pushed the brendon/config-default branch from ea87f1b to c890bf1 Compare June 5, 2024 21:14
@stappersg
Copy link
Contributor

(Here not the project lead, some random Joe)
I see only lines moved, I don't see any improvement.
Most likely due me not understanding the "I'd like to use Config::default in my own code when constructing a Config. Let's implement the Default trait so that others can use it without wrapping our struct due to ownership rules." commit message.

@BKDaugherty
Copy link
Contributor Author

BKDaugherty commented Jun 5, 2024

Most likely due me not understanding the "I'd like to use Config::default in my own code when constructing a Config. Let's implement the Default trait so that others can use it without wrapping our struct due to ownership rules." commit message.

Ah. @stappersg, no worries! Happy to explain :)

It's fairly common in Rust for types to implement certain Traits, (or interfaces in other languages), for standard behaviors. One of those behaviors is constructing a default representation of a type. Check out the docs on the trait here: https://doc.rust-lang.org/std/default/trait.Default.html

I typically do this when I'd like to create a config with specific values, but I don't really care about the other values.

Let's take for example someone wanting to run their TFTP server on the IPV6 localhost (::1), but they want to just spin up the rest of the config to the default values. (Say for example, they don't know that the port should be 69, or don't want to duplicate that constant in their own code.) They can use the following syntax

let config = Config {
  ip_address: IpAddr::V6(Ipv6Addr::LOCALHOST),
  ..Config::default()
};

And rely on the default implementation to do the rest.

It's possible to do something similar to the above with the existing Config::new method, but a little awkward, and unexpected compared to the rest of the ecosystem, which relies on the standard library's trait.

    let config = Config {
        ip_address: IpAddr::V6(Ipv6Addr::LOCALHOST),
	..Config::new([].into_iter()).expect("Config building with no args should be fine, and my application depends on that")
    };

@altugbakan
Copy link
Owner

Looks great initially, I’ll take a closer look when I get home. Thank you for your addition!

Copy link
Owner

@altugbakan altugbakan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, I just only have one small styling request.

src/config.rs Outdated Show resolved Hide resolved
I'd like to use Config::default in my own code when constructing a
`Config`. Let's implement the Default trait so that others can use it
without wrapping our struct due to ownership rules.
@BKDaugherty BKDaugherty force-pushed the brendon/config-default branch from c890bf1 to 5b49110 Compare June 6, 2024 16:49
Copy link
Owner

@altugbakan altugbakan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@altugbakan altugbakan merged commit 427e207 into altugbakan:main Jun 6, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants