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

Skip documentation files without ``` when running markdown tests. #42437

Merged
merged 1 commit into from
Jun 6, 2017

Conversation

Mark-Simulacrum
Copy link
Member

This should reduce the 'running 0 tests' noise in builds, and I believe this is a good heuristic for us to use.

cc @rust-lang/docs -- do we use the indented format for code blocks anywhere? Will we? If so, we shouldn't do this.

r? @alexcrichton

This should reduce the 'running 0 tests' noise in builds, and is a good
heuristic for us to use.
@GuillaumeGomez
Copy link
Member

Why not? :)

@kennytm
Copy link
Member

kennytm commented Jun 5, 2017

It should be safe to go ahead. But there is no harm to check also \n\n      as well.

Edit removed irrelevant info.

@Mark-Simulacrum
Copy link
Member Author

I'm not sure I follow -- those presumably wouldn't be present in markdown files? Are they indications of code blocks? It doesn't feel to me like they are, but I could be wrong.

@alexcrichton
Copy link
Member

Looks reasonable to me! My only worry is, as you commented above, not catching indented blocks or those separated by ~~~ (that's historical, right?).

That being said if our style is to only use backticks (which I think it is) then this looks good to me.

@Mark-Simulacrum
Copy link
Member Author

Yeah, I want @steveklabnik to comment to make sure that we're good with just backticks, but after that I think we'll be good to go. I initially wanted to put this sort of functionality into rustdoc directly (calling it with multiple files which it'd process separately but print together in one list) but that seemed more complicated and I decided to wait on that at least until the next rustdoc.

Copy link
Member

@steveklabnik steveklabnik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not opposed to this idea, but it also doesn't fully cover all the cases yet.

let mut file = t!(File::open(markdown));
let mut contents = String::new();
t!(file.read_to_string(&mut contents));
if !contents.contains("```") {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this isn't the only way there are code blocks though; for example, four spaces is also considered a code block

@steveklabnik
Copy link
Member

That being said if our style is to only use backticks (which I think it is) then this looks good to me.

Yeah, it is; if everyone else is okay with just checking backticks, then I won't let the perfect be the enemy of the good 😄

@Mark-Simulacrum
Copy link
Member Author

I could add something like \n followed by 4 spaces, but I feel like that heuristic wouldn't work too well due to indentation. I'm open to adding it if we want to though.

@carols10cents carols10cents added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Jun 5, 2017
@Mark-Simulacrum Mark-Simulacrum 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 Jun 5, 2017
@alexcrichton
Copy link
Member

@steveklabnik the failure mode here will be that if you write a doc example or a doc block without three ticks in a markdown file then it won't get tested (in just rust-lang/rust during ./x.py test). Are you ok with that or do you think we should catch more cases?

@Mark-Simulacrum this is primarily done to cut down on all the output generated, right? Do you know how many files end up having 0 tests?

@Mark-Simulacrum
Copy link
Member Author

Mark-Simulacrum commented Jun 5, 2017

I believe 417 files would be skipped with this check.

rg --files-without-match '```' src/doc | wc -l

@steveklabnik
Copy link
Member

@steveklabnik the failure mode here will be that if you write a doc example or a doc block without three ticks in a markdown file then it won't get tested (in just rust-lang/rust during ./x.py test). Are you ok with that or do you think we should catch more cases?

I think virtually everything uses the triple ticks, so it should be fine. Gonna bookmark this comment for someday when this randomly bites us, though 😆

@alexcrichton
Copy link
Member

Heh ok! Let's get that sweet sweet thousands-of-less-lines-of-output from travis then!

@bors: r+

@bors
Copy link
Contributor

bors commented Jun 5, 2017

📌 Commit dd1d75e has been approved by alexcrichton

@Mark-Simulacrum Mark-Simulacrum added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 5, 2017
@bors
Copy link
Contributor

bors commented Jun 6, 2017

⌛ Testing commit dd1d75e with merge e1293ec...

bors added a commit that referenced this pull request Jun 6, 2017
Skip documentation files without ``` when running markdown tests.

This should reduce the 'running 0 tests' noise in builds, and I believe this is a good heuristic for us to use.

cc @rust-lang/docs -- do we use the indented format for code blocks anywhere? Will we? If so, we shouldn't do this.

r? @alexcrichton
@bors
Copy link
Contributor

bors commented Jun 6, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing e1293ec to master...

@bors bors merged commit dd1d75e into rust-lang:master Jun 6, 2017
frewsxcv added a commit to frewsxcv/rust that referenced this pull request Jun 7, 2017
…crichton

Skip printing for skipped doc tests.

Followup to rust-lang#42437 to further reduce noise.

r? @alexcrichton
bors added a commit that referenced this pull request Jul 10, 2017
Test src/doc once more

This was accidentally broken in #42437 since we filtered too early to recurse into sub-directories.

In theory, @bors p=10

r? @alexcrichton
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants