Skip to content

add documentation from doc(include) to analysis data #47496

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 2 commits into from
Feb 6, 2018

Conversation

QuietMisdreavus
Copy link
Member

cc #44732

Currently save-analysis only loads docs from plain doc comments and doc attributes. Since #[doc(include="filename.md")] doesn't create a plain doc attribute when it loads the file, we need to be sure to pick up this info for the analysis data.

@rust-highfive
Copy link
Contributor

r? @estebank

(rust_highfive has picked a reviewer for you, use r? to override)

@estebank
Copy link
Contributor

Should this have some kind of test? I don't know how to test it though. r=me once ci finishes.

@QuietMisdreavus
Copy link
Member Author

Before pushing, i had a quick look through the tests in src/test and didn't see any that invoked save-analysis, so i'm not sure what the best way to test it would be. I don't know whether/how the rest of save-analysis is tested, otherwise i would try to slot it into there.

@estebank
Copy link
Contributor

I figured that might be the case :-/

@nrc are you aware of any testing for save-analysis?

@kennytm kennytm added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 17, 2018
@nrc
Copy link
Member

nrc commented Jan 18, 2018

There are a couple of smoke tests in run-make/save-analysis or something similar which basically only test that the compiler doesn't crash. However, it is useful to add things like this to one of those tests.

@carols10cents carols10cents added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 22, 2018
@shepmaster
Copy link
Member

Ping from triage @QuietMisdreavus ! We haven't heard from you in a week; will you be able to address the recent comments soon?

@QuietMisdreavus
Copy link
Member Author

Whoops, forgot about this one. I guess the way forward would be to just add something with #[doc(include="file.md")] into the save-analysis test to make sure it doesn't crash? If that's all, then i can get that up shortly. (Assuming i can wrangle my system into running the test. >_>)

@pietroalbini pietroalbini added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 5, 2018
@pietroalbini
Copy link
Member

@estebank ping from triage!

@estebank
Copy link
Contributor

estebank commented Feb 5, 2018

@bors r+ rollup

@bors
Copy link
Collaborator

bors commented Feb 5, 2018

📌 Commit 5a350c1 has been approved by estebank

kennytm added a commit to kennytm/rust that referenced this pull request Feb 5, 2018
…estebank

add documentation from doc(include) to analysis data

cc rust-lang#44732

Currently save-analysis only loads docs from plain doc comments and doc attributes. Since `#[doc(include="filename.md")]` doesn't create a plain doc attribute when it loads the file, we need to be sure to pick up this info for the analysis data.
bors added a commit that referenced this pull request Feb 6, 2018
Rollup of 10 pull requests

- Successful merges: #46030, #47496, #47543, #47704, #47753, #47807, #47948, #47959, #48003, #48007
- Failed merges:
@bors bors merged commit 5a350c1 into rust-lang:master Feb 6, 2018
@QuietMisdreavus QuietMisdreavus deleted the rls-doc-include branch February 26, 2018 23:20
# for free to join this conversation on GitHub. Already have an account? # 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.

9 participants