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

Add Builder::whitelist_var; deprecate Builder::whitelisted_var #986

Closed
3 tasks
fitzgen opened this issue Sep 13, 2017 · 8 comments
Closed
3 tasks

Add Builder::whitelist_var; deprecate Builder::whitelisted_var #986

fitzgen opened this issue Sep 13, 2017 · 8 comments

Comments

@fitzgen
Copy link
Member

fitzgen commented Sep 13, 2017

The CLI flag is --whitelist-var, so we should be consistent and remove the "ed" from the builder method.

  • Rename Builder::whitelisted_var to Builder::whitelist_var
  • Add Builder::whitelisted_var that delegates to whitelist_var
  • Mark whitelisted_var as #[deprecated = "use whitelist_var instead"]
@highfive
Copy link

Hi! If you have any questions regarding this issue, feel free to make a comment here, or ask it in the #servo channel in IRC.

If you intend to work on this issue, then add @highfive: assign me to your comment, and I'll assign this to you. 😄

@aeleos
Copy link
Contributor

aeleos commented Sep 13, 2017

@highfive: assign me

@highfive
Copy link

Hey @aeleos! Thanks for your interest in working on this issue. It's now assigned to you!

@fitzgen
Copy link
Member Author

fitzgen commented Sep 13, 2017

@aeleos don't hesitate to ping me if you have any questions :)

@aeleos
Copy link
Contributor

aeleos commented Sep 13, 2017

@fitzgen I do have one question, is there any recommended place to put the old deprecated function? I am thinking right next to it is probably best, but at the bottom could also work.

@aeleos
Copy link
Contributor

aeleos commented Sep 14, 2017

@fitzgen I lied I do have another question, I ended up with this code

/// Whitelist the given variable so that it (and all types that it
/// transitively refers to) appears in the generated bindings. Regular
/// expressions are supported.
pub fn whitelist_var<T: AsRef<str>>(mut self, arg: T) -> Builder {
    self.options.whitelisted_vars.insert(arg);
    self
}

/// Whitelist the given variable.
///
/// Deprecated: use whitelist_var instead.
#[deprecated = "use whitelist_var instead"]
pub fn whitelisted_var<T: AsRef<str>>(self, arg: T) -> Builder {
    self.whitelist_var(arg)
}

The compiler said that I didn't need mut self on the deprecated function, and just self was fine. It compiles but it strikes me as odd, does it make sense to you or is there a problem I am not seeing

@fitzgen
Copy link
Member Author

fitzgen commented Sep 14, 2017

I am thinking right next to it is probably best, but at the bottom could also work.

Right next to it is fine.

The compiler said that I didn't need mut self on the deprecated function, and just self was fine. It compiles but it strikes me as odd, does it make sense to you or is there a problem I am not seeing

There is no problem you're not seeing.

To call a method that takes self by value, you don't need mut, but to modify any of self's fields locally you need mut. Its a bit funky. This sparked a huge discussion right before Rust 1.0, but ultimately we stayed with what we currently have: https://www.reddit.com/r/rust/comments/25i544/babysteps_focusing_on_ownership_or_removing_let/

@aeleos
Copy link
Contributor

aeleos commented Sep 14, 2017

@fitzgen ah thanks for the info. I created a PR, hopefully I did it correctly.

bors-servo pushed a commit that referenced this issue Sep 14, 2017
Make whitelisted_var consistant with CLI flags

Closes #986

This is my first time contributing to rust so hopefully I did everything right, otherwise let me know.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants