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(nickel): add options to Nickel, allowing the user to disable the messages output on listen #277
Conversation
…he messages output on listen
/// server.set_options (options); | ||
/// # } | ||
/// ``` | ||
pub fn default_options() -> Options { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any benefit to this over implementing Default
and having Options::default()
instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess not. I don't have a lot of knowledge in Rust yet, I was not aware of the Default trait.
@ixjf What do you think about making the let server = ..;
server.options_mut()
.print_on_listen(false); |
It's definitely cleaner. I don't see why not. |
@Ryman How would you go about implementing accessors, since you don't have function overloading in Rust? Have different functions get_print_on_listen and set_print_on_listen, or print_on_listen_mut/print_on_listen? What about each setting being part of an enum, and having a single set/get function? |
I think for now though we don't need to expose We can just keep the flag as a private detail for now and see if anybody has a reason to read it other than
Yeah, different functions would be the usual strategy. cc @cburgdorf |
The only problem I see with that is that if you eventually want to add other options, you'd either have to stick them all in Nickel, have them in multiple places, or break compatibility. |
You're right, it's a pity we don't have stability markers outside of If we end up with something similar to the current code in future then |
Sorry, I'm having a hard time following (it's moment's like this when I realize that I'm still a Rust noob). Where is the problem with server.options_mut()
.print_on_listen(false); With the |
I don't have a lot of knowledge in Rust either, but I don't think that it would be possible to do cleanly as you would in a language like C++ where function overloading is supported. The problem with that approach is that you can't make print_on_listen both a setter and a getter, while allowing setter calls to be chained (or really, you simply can't make it both a setter and a getter - unless you make it a field instead of a function, but then it can't be chained, which was the whole point). @Ryman Fair enough. Is it convention that Rust libraries use 0.x for non-dev releases, or are all libraries simply not stable enough? If you follow semantic versioning (semver.org), then 0.x means unstable builds, which means you can still break compatibility without worrying too much. That's not the case, though, is it? Either way, I'll make the required changes to the commit and squash them together. |
Only if exposing
Not sure what you mean by non-dev releases, but yes a lot of libs seem to be sticking with 0.x holding out for whatever rust feature/dependency they require to get the feature they want for 1.0! We've been trying to follow this although we've been quite slow at bumping actual new releases to crates.io (the git repo itself should be considered very unstable). We've been bumping the minor revision each time we've been breaking something substantial as with most of our dependencies, e.g. you can depend on 0.6.x to hopefully not break you if you depend on 0.6.0, but 0.7.0 can be a huge change (due to pre 1.0 experimentation) :) |
Ok, I had to get my hand on some code to follow and now I actually see what's the problem. It's tricky if you wand to have chaining, too. I thought you could do something like this but it also doesn't work and is most likely nonsense that just seemed to be a good idea in my overly tired brain ;) struct Options {
some_bool: bool,
some_string: String
}
struct Modifier<'options, T: 'options> {
options: &'options Options,
val: & 'options mut T
}
impl<'options,T> Modifier<'options, T> {
fn set (&mut self, val: T) -> & 'options mut Options {
self.val = val;
&mut self.options
}
}
impl Options {
fn some_bool_mut (&mut self) -> Modifier<bool> {
Modifier {
options: self,
val: self.some_bool
}
}
fn some_string_mut (&mut self) -> Modifier<String> {
Modifier {
options: self,
val: self.some_string
}
}
fn default () -> Options {
Options {
some_bool: true,
some_string: "Foo".to_owned()
}
}
}
fn main() {
println!("Hello, world!");
let options = Options::default();
options
.some_bool_mut()
.set(false)
.some_string_mut()
.set("bar".to_owned());
} |
@cburgdorf See https://github.com/reem/rust-modifier for a more full-blown implementation of what you've described (we're using that for some parts of |
I don't really like it that you have calls to different objects in the chain (especially in different lines, however, that's really just personal preference). |
@Ryman haha, great! So it actually made at least a little bit of sense. I've stumbled over this in our nickel code before but never made the stretch to understand what it actually is :D It's another thing to put on my list of things that I should learn about in Rust land. Well, if we use it for |
@ixjf that just seems to be a weakness in the crappy code that I made up. With the real |
Ah, yes. That is actually similar to what I was suggesting earlier. |
Just seems less 'obvious' for users, but I guess this feature is probably only going to be used by people who know what they're doing. I don't have anything much against it, if you guys are happy to roll with this then that's good enough for me 👍 |
Perhaps this should be left for when more options are to be added? It's getting too complicated for such a simple thing. |
Yes, I agree. We can go for the simple solution now and reach for |
☔ The latest upstream changes (presumably #280) made this pull request unmergeable. Please resolve the merge conflicts. |
So - does this stay as it is and this is dealt with later, or do we move forward and merge this (after making the changes we talked about)? |
I'm happy enough for the simple solution (the method on nickel) to land for now, if you wanted to add a this is an unstable method to the doc comment then that would be pretty great too. Perhaps we should also name it |
Relevant to this discussion: tokio-rs/mio@8b84f33 |
This is the same that we discussed before, but it wouldn't allow getters. But to be honest, if I think about it, I'm not sure there's any option even worth getting by an application? |
@ixjf Yeah, the link is very similar to how we discussed, which is good. It's just an example to see that this style is quite idiomatic (expert rust developer, prolific rust project).
I think we should leave off getters until someone presents a need for it :) Then we can discuss the problem with more understanding of requirements. |
@ixjf Any update on this? |
@ixjf Is this still in the works? |
Closing as we've merged #310 which is based on this. Thanks for the initial work and discussion on this feature! |
This is in relation to the pull request #124 - it adds two methods to Nickel - a static default_options, and set_options. The only option available is output_on_listen, which when disabled, won't display "Listening on x.x.x.x:xxxx/Press Ctrl+C to shutdown the server" (it is enabled by default, which is the current behavior). This could be extended later on for whatever other options.