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(nickel): add options to Nickel, allowing the user to disable the messages output on listen #277

Closed
wants to merge 1 commit into from

Conversation

ixjf
Copy link
Contributor

@ixjf ixjf commented Sep 5, 2015

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.

@ixjf ixjf changed the title feat(nickel): added options to Nickel, allowing the user to disable the messages output on listen feat(nickel): add options to Nickel, allowing the user to disable the messages output on listen Sep 5, 2015
/// server.set_options (options);
/// # }
/// ```
pub fn default_options() -> Options {
Copy link
Member

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?

Copy link
Contributor Author

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.

@Ryman
Copy link
Member

Ryman commented Sep 5, 2015

@ixjf What do you think about making the Options have a builder style api?

let server = ..;
server.options_mut()
      .print_on_listen(false);

@ixjf
Copy link
Contributor Author

ixjf commented Sep 5, 2015

It's definitely cleaner. I don't see why not.

@ixjf
Copy link
Contributor Author

ixjf commented Sep 5, 2015

@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?

@Ryman
Copy link
Member

Ryman commented Sep 5, 2015

I think for now though we don't need to expose getters and see if a need appears? Perhaps we should raise print_on_listen(bool) to be on Nickel instead and we can forget about Options until it has more purpose? (Perhaps in relation to #161)

We can just keep the flag as a private detail for now and see if anybody has a reason to read it other than Nickel itself (which can access the private data)? (I'm unsure about the best approach here, but it therefore it seems safer to expose less and do the simplest thing)

since you don't have function overloading in Rust

Yeah, different functions would be the usual strategy.

cc @cburgdorf

@ixjf
Copy link
Contributor Author

ixjf commented Sep 5, 2015

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.

@Ryman
Copy link
Member

Ryman commented Sep 5, 2015

You're right, it's a pity we don't have stability markers outside of rustc. I'm not too concerned with breaking very minor things, it should be easy enough to migrate off a simple flag like this, especially if we haven't given enough api to let people to depend on reading the value.

If we end up with something similar to the current code in future then Nickel::print_on_listen(bool) can just hopefully just wrap self.set_options(..).

@cburgdorf
Copy link
Member

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 options_mut approach, we would actually have get/set behavior available, no?

@ixjf
Copy link
Contributor Author

ixjf commented Sep 5, 2015

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.

@Ryman
Copy link
Member

Ryman commented Sep 5, 2015

@cburgdorf

With the options_mut approach, we would actually have get/set behavior available, no?

Only if exposing Options fields publicly would you get get, which would break code matching on it pretty easily if fields are added (so adding all the extra code doesn't seem worth it to me, as we don't know how the future will look here).

Is it convention that Rust libraries use 0.x for non-dev releases, or are all libraries simply not stable enough?

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) :)

@cburgdorf
Copy link
Member

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());
}

@Ryman
Copy link
Member

Ryman commented Sep 5, 2015

@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 Response, see set). I'm not sure if this single option warrants going all-in on adding that to Nickel, but perhaps I could be convinced?

@ixjf
Copy link
Contributor Author

ixjf commented Sep 5, 2015

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).

@cburgdorf
Copy link
Member

@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 Response already and it's not a super heavy and hairy thing to introduce to this part of the code, why not?

@cburgdorf
Copy link
Member

@ixjf that just seems to be a weakness in the crappy code that I made up. With the real rust-modifier thing used, it looks much cleaner: http://docs.nickel.rs/nickel/struct.Response.html#method.set

@ixjf
Copy link
Contributor Author

ixjf commented Sep 5, 2015

Ah, yes. That is actually similar to what I was suggesting earlier.

@Ryman
Copy link
Member

Ryman commented Sep 5, 2015

Well, if we use it for Response already and it's not a super heavy and hairy thing to introduce to this part of the code, why not?

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 👍

@ixjf
Copy link
Contributor Author

ixjf commented Sep 7, 2015

Perhaps this should be left for when more options are to be added? It's getting too complicated for such a simple thing.

@cburgdorf
Copy link
Member

Yes, I agree. We can go for the simple solution now and reach for rust-modifier when we have more options.

@homu
Copy link
Contributor

homu commented Sep 10, 2015

☔ The latest upstream changes (presumably #280) made this pull request unmergeable. Please resolve the merge conflicts.

@ixjf
Copy link
Contributor Author

ixjf commented Sep 10, 2015

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)?

@Ryman
Copy link
Member

Ryman commented Sep 10, 2015

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 set_print_on_listen(bool) as that seems more akin to common style.

@Ryman
Copy link
Member

Ryman commented Sep 11, 2015

Relevant to this discussion: tokio-rs/mio@8b84f33

@ixjf
Copy link
Contributor Author

ixjf commented Sep 11, 2015

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?

@Ryman
Copy link
Member

Ryman commented Sep 11, 2015

@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'm not sure there's any option even worth getting by an application?

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.

@Ryman
Copy link
Member

Ryman commented Nov 18, 2015

@ixjf Any update on this?

@euclio
Copy link
Contributor

euclio commented Jan 1, 2016

@ixjf Is this still in the works?

@Ryman
Copy link
Member

Ryman commented Mar 10, 2016

Closing as we've merged #310 which is based on this.

Thanks for the initial work and discussion on this feature!

@Ryman Ryman closed this Mar 10, 2016
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.

None yet

5 participants