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 pub visibility for methods as well #44554

Merged
merged 1 commit into from
Sep 23, 2017

Conversation

GuillaumeGomez
Copy link
Member

@QuietMisdreavus
Copy link
Member

r=me pending travis

@QuietMisdreavus
Copy link
Member

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Sep 14, 2017

📌 Commit 42a0286 has been approved by QuietMisdreavus

@ollie27
Copy link
Member

ollie27 commented Sep 14, 2017

The head_len part just above this will need to be updated as well. Also this could do with some tests.

@QuietMisdreavus
Copy link
Member

@bors r-

Good catch, @ollie27. I hesitated asking for some tests, but it is a good idea.

@GuillaumeGomez
Copy link
Member Author

@ollie27: Good catch!

@GuillaumeGomez
Copy link
Member Author

However I have no idea how to test it...

@ollie27
Copy link
Member

ollie27 commented Sep 14, 2017

I'm not sure how to add tests for the indentation but a simple test like the following would be useful:

#![crate_name = "foo"]

pub struct Foo;

impl Foo {
    // @has 'foo/struct.Foo.html' '//code' 'pub fn f()'
    pub fn f() {}
}

@QuietMisdreavus
Copy link
Member

I rummaged through the flags available to compiler tests and it looks like we can't add flags to rustdoc (to enable rendering private items), so for now that will have to do.

@ollie27
Copy link
Member

ollie27 commented Sep 14, 2017

@QuietMisdreavus do you mean like

// compile-flags: --no-defaults --passes collapse-docs --passes unindent-comments --passes strip-priv-imports
?

@QuietMisdreavus
Copy link
Member

Um, yes, exactly that. I didn't realize that the compile-flags worked like that. So yeah, that's what i had in mind to be able to test rendering pub fn and fn distinctly.

@GuillaumeGomez
Copy link
Member Author

Oh damn, I forgot to add the test file! Now I see what you're talking about...

@GuillaumeGomez
Copy link
Member Author

I added the test.

@alexcrichton alexcrichton added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 14, 2017
@bors
Copy link
Contributor

bors commented Sep 15, 2017

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

@GuillaumeGomez GuillaumeGomez force-pushed the add-missing-pub branch 2 times, most recently from 795c559 to 7800fae Compare September 18, 2017 16:09
@QuietMisdreavus
Copy link
Member

Could you add another bit to the test where it renders a non-pub function without "pub"? You can disable the strip-private pass using the suggestion from #44554 (comment) above.

@QuietMisdreavus
Copy link
Member

(the test got updated to include a non-pub method, and travis passed, so:)

@bors r+

@bors
Copy link
Contributor

bors commented Sep 19, 2017

📌 Commit 203d71f has been approved by QuietMisdreavus

GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Sep 21, 2017
…uietMisdreavus

Add pub visibility for methods as well

Fixes rust-lang#44527.

r? @QuietMisdreavus
frewsxcv added a commit to frewsxcv/rust that referenced this pull request Sep 23, 2017
…uietMisdreavus

Add pub visibility for methods as well

Fixes rust-lang#44527.

r? @QuietMisdreavus
bors added a commit that referenced this pull request Sep 23, 2017
Rollup of 14 pull requests

- Successful merges: #44554, #44648, #44658, #44712, #44717, #44726, #44745, #44746, #44749, #44759, #44770, #44773, #44776, #44778
- Failed merges:
@bors bors merged commit 203d71f into rust-lang:master Sep 23, 2017
@GuillaumeGomez GuillaumeGomez deleted the add-missing-pub branch September 23, 2017 11:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants