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.

Sign up for GitHub

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

Jump to bottom

don't let rustdoc get confused by text "fn main" in a line comment #44713

Merged
merged 1 commit into from
Sep 27, 2017
Merged

don't let rustdoc get confused by text "fn main" in a line comment #44713

merged 1 commit into from
Sep 27, 2017

Conversation

zackmdavis
Copy link
Member

@zackmdavis zackmdavis commented Sep 20, 2017 •
edited
Loading 


![rustdoc_fn_main](https://user-images.githubusercontent.com/1076988/30630993-9aeecc4a-9d97-11e7-8e56-2b973f23f683.png)

r? @QuietMisdreavus

QuietMisdreavus
QuietMisdreavus reviewed Sep 20, 2017
View reviewed changes
Copy link
Member

@QuietMisdreavus QuietMisdreavus left a comment

Choose a reason for hiding this comment

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

Thanks so much, i love it! Just one little nit and this should be good to go!

src/librustdoc/test.rs Outdated
line
}
})
.find(|code| code.contains("fn main")).is_some();
Copy link
Member

Choose a reason for hiding this comment

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

Could you make this line .any(|code| code.contains("fn main")) instead? That's equivalent and removes one level of method chaining.

Copy link
Member

@GuillaumeGomez GuillaumeGomez Sep 20, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

This is indeed a great improvement. I just have a concern on this (or the previous check):

Imagine you wrote

fn    main

(for some reasons...). Or if you write "fn main" in a string (for some other reasons...).

Copy link
Member

Choose a reason for hiding this comment

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

True, this isn't a "complete" fix, but it's still a start. A "proper" fix would involve throwing libsyntax or the like at the doctest and scanning that for a function called "main". But for now, this is much better than we have at the moment.

Copy link
Member

Choose a reason for hiding this comment

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

I am thinking about a partial fix involving a stupid parser creating basic tokens and all. But still bigger than this.

Copy link
Member

Choose a reason for hiding this comment

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

I would prefer that rustdoc instead gain something like allowing lib where we have rust (i.e., in the language part after "```") today. The more work we do for the "common case" the worse rustdoc's performance on codebases with large amounts of doc tests gets.

Copy link
Member

Choose a reason for hiding this comment

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

Agreed. There's no ideal solution in here.

@aidanhs aidanhs added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. 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 Sep 20, 2017
@zackmdavis zackmdavis force-pushed the fn_main_in_a_comment_in_rustdoc_breaks_tests branch from 10d418a to 0b2bac4 Compare September 22, 2017 01:38
@zackmdavis
Copy link
Member Author

Force-push addresses @QuietMisdreavus's find/any comment. I regret that the Actual Parsing that would be required to fix similar edge-cases is out of scope for this pull request.

@carols10cents carols10cents 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 Sep 25, 2017
@QuietMisdreavus
Copy link
Member

Sorry for letting this get stale! To get this put in, i won't force you to wrangle libsyntax to do Actual Parsing. I'll go ahead and accept this PR, but i want to leave #21299 open so we can track a Proper Solution for it.

To that end, can you add a comment "FIXME(#21299): Replace this with libsyntax-based check" to the change?

@zackmdavis zackmdavis force-pushed the fn_main_in_a_comment_in_rustdoc_breaks_tests branch from 0b2bac4 to 9f68d62 Compare September 26, 2017 23:01
@zackmdavis
Copy link
Member Author

(updated)

@QuietMisdreavus
Copy link
Member

@bors r+

Thanks so much! Sorry about the delay!

@bors
Copy link
Contributor

bors commented Sep 27, 2017

📌 Commit 9f68d62 has been approved by QuietMisdreavus

@bors
Copy link
Contributor

bors commented Sep 27, 2017

⌛ Testing commit 9f68d62 with merge 1fd3a42...

bors added a commit that referenced this pull request Sep 27, 2017
@bors
Auto merge of #44713 - zackmdavis:fn_main_in_a_comment_in_rustdoc_bre…
1fd3a42 
…aks_tests, r=QuietMisdreavus

don't let rustdoc get confused by text "fn main" in a line comment

~~~Resolves~~~ (edited) partially addresses #21299.

![rustdoc_fn_main](https://user-images.githubusercontent.com/1076988/30630993-9aeecc4a-9d97-11e7-8e56-2b973f23f683.png)

r? @QuietMisdreavus
@bors
Copy link
Contributor

bors commented Sep 27, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: QuietMisdreavus
Pushing 1fd3a42 to master...

@bors bors merged commit 9f68d62 into rust-lang:master Sep 27, 2017
@QuietMisdreavus QuietMisdreavus mentioned this pull request Sep 29, 2017
Closed
@zackmdavis zackmdavis deleted the fn_main_in_a_comment_in_rustdoc_breaks_tests branch January 13, 2018 07:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@QuietMisdreavus QuietMisdreavus QuietMisdreavus left review comments

@Mark-Simulacrum Mark-Simulacrum Mark-Simulacrum left review comments

@GuillaumeGomez GuillaumeGomez GuillaumeGomez left review comments

Assignees

@QuietMisdreavus QuietMisdreavus

Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@zackmdavis @QuietMisdreavus @bors @GuillaumeGomez @Mark-Simulacrum @carols10cents @aidanhs