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

"Generate getter/setter" is too opinionated #12273

Closed
jonas-schievink opened this issue May 16, 2022 · 2 comments · Fixed by #12296
Closed

"Generate getter/setter" is too opinionated #12273

jonas-schievink opened this issue May 16, 2022 · 2 comments · Fixed by #12296
Assignees
Labels
A-assists C-feature Category: feature request S-actionable Someone could pick this issue up and work on it right now

Comments

@jonas-schievink
Copy link
Contributor

jonas-schievink commented May 16, 2022

Given

struct S {
    a$0: u8,
}

the "Generate getter/setter" assist can generate the following 3 methods:

impl S {
    /// Get a mutable reference to the s's a.
    #[must_use]
    fn a_mut(&mut self) -> &mut u8 {
        &mut self.a
    }

    /// Set the s's a.
    fn set_a(&mut self, a: u8) {
        self.a = a;
    }

    /// Get the s's a.
    #[must_use]
    fn a(&self) -> u8 {
        self.a
    }
}

However, there's no configuration for how these should look, so we impose some odd idiosyncrasies on users, such as:

  • Generated docs require manual editing, which can arguably be worse than no docs because it creates more work for the programmer (the docs are also spelled weirdly) The assist no longer generates docs
  • We always add #[must_use], which is "weird" – it makes Rust even more boilerplate-heavy and adds visual noise.
  • We never add #[inline] but users might want that, at least when the type in question has no generic type/const parameters.

This leads to people (me) not using the assist because their handwritten code would differ too much from what the assist generates.

We should add a config section for this group of assists and make each of these points configurable. The defaults should possibly be changed as well.

@jonas-schievink jonas-schievink added S-actionable Someone could pick this issue up and work on it right now A-assists C-feature Category: feature request labels May 16, 2022
@jonas-schievink
Copy link
Contributor Author

Since we already have an assist for generating docs, I think that part can be removed entirely here.

@flodiebold
Copy link
Member

flodiebold commented May 16, 2022

I agree. I would probably argue these things should be separate assists, though since we have them now it would also be ok to have them optional.

For docs, it would be nice if the existing assist proposed the default description for getters/setters, maybe even automatically selected so it's easy to change. (It seems to special-case constructors already.)

@jonas-schievink jonas-schievink self-assigned this May 16, 2022
bors added a commit that referenced this issue May 16, 2022
…=jonas-schievink

feat: Handle getters and setters in documentation template assist

The assist can now turn this:

```rust
pub struct S;
impl S {
    pub fn data_mut$0(&mut self) -> &mut [u8] { &mut [] }
}
```

into

```rust
pub struct S;
impl S {
    /// Returns a mutable reference to the data.
    ///
    /// # Examples
    ///
    /// ```
    /// use test::S;
    ///
    /// let mut s = ;
    /// assert_eq!(s.data_mut(), );
    /// assert_eq!(s, );
    /// ```
    pub fn data_mut(&mut self) -> &mut [u8] { &mut [] }
}
```

And similarly for by-value or immutable getters, and for setters. Previously the intro line would be empty.

This PR also removes the documentation generation function from the "Generate getter/setter" assist, since that is better handled by applying the 2 assists in sequence. cc #12273
@jonas-schievink jonas-schievink changed the title "Generate getter/setter" should be more configurable "Generate getter/setter" is too opinionated May 16, 2022
@bors bors closed this as completed in 52b69fa May 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-assists C-feature Category: feature request S-actionable Someone could pick this issue up and work on it right now
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants