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

Refactor test_lang_string_parse to make it clearer #80013

Merged
merged 1 commit into from
Dec 14, 2020
Merged

Refactor test_lang_string_parse to make it clearer #80013

merged 1 commit into from
Dec 14, 2020

Conversation

poliorcetics
Copy link
Contributor

@poliorcetics poliorcetics commented Dec 13, 2020 •
edited
Loading

Follows #79454 (comment)

A small PR made to refactor a test in rustdoc that was becoming unwieldy.

@rustbot label T-rustdoc
r? @jyn514

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 13, 2020
@rustbot rustbot added the T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. label Dec 13, 2020
@jyn514 jyn514 added A-markdown-parsing Area: Markdown parsing for doc-comments A-testsuite Area: The testsuite used to check the correctness of rustc C-cleanup Category: PRs that clean code up or issues documenting cleanup. labels Dec 13, 2020
@jyn514
Copy link
Member

jyn514 commented Dec 13, 2020

Looks great! Thanks so much :)

@bors r+ rollup

Another thing I noticed is that LangString::all_false is not actually all_false - rust will return true. It would be nice to rename it to default() at some point (and maybe add impl Default for LangString while we're at it). No need to change it here, though.

@bors
Copy link
Contributor

bors commented Dec 13, 2020

📌 Commit ec0f1d7 has been approved by jyn514

@bors bors 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 Dec 13, 2020
@poliorcetics
Copy link
Contributor Author

Another thing I noticed is that LangString::all_false is not actually all_false - rust will return true. It would be nice to rename it to default() at some point (and maybe add impl Default for LangString while we're at it). No need to change it here, though.

Yeah, I noticed that too when modifying this test in the original PR, there is a comment about historical reasons I believe. I would like to change it too but I'm waiting on #79454, else I'll have to rebase all the time, with all the problems it brings because git does not like rebasing.

@jyn514
Copy link
Member

jyn514 commented Dec 13, 2020

Well, I'd expect #79454 to be blocked another long while unfortunately ... but this refactor isn't urgent, I can just open an issue in the meantime.

@poliorcetics
Copy link
Contributor Author

Ping me on it, I have already added it to my todo-list 😄

@jyn514 jyn514 mentioned this pull request Dec 13, 2020
Closed
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Dec 14, 2020
@Dylan-DPC
Rollup merge of rust-lang#80013 - poliorcetics:rustdoc-test-refactor,…
549937c 
… r=jyn514

Refactor test_lang_string_parse to make it clearer

Follows rust-lang#79454 (comment)

A small PR made to refactor a test in rustdoc that was becoming unwieldy.

`@rustbot` label T-rustdoc
r? `@jyn514`
@Dylan-DPC-zz Dylan-DPC-zz mentioned this pull request Dec 14, 2020
Closed
@GuillaumeGomez GuillaumeGomez mentioned this pull request Dec 14, 2020
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 14, 2020
@bors
Auto merge of rust-lang#80024 - GuillaumeGomez:rollup-rqd46ko, r=Guil…
1f7762b 
…laumeGomez

Rollup of 3 pull requests

Successful merges:

 - rust-lang#79918 (doc(array,vec): add notes about side effects when empty-initializing)
 - rust-lang#79936 (Fix item name display on mobile)
 - rust-lang#80013 (Refactor test_lang_string_parse to make it clearer)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 2169094 into rust-lang:master Dec 14, 2020
@rustbot rustbot added this to the 1.50.0 milestone Dec 14, 2020
@poliorcetics poliorcetics deleted the rustdoc-test-refactor branch December 14, 2020 17:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@jyn514 jyn514

Labels
A-markdown-parsing Area: Markdown parsing for doc-comments A-testsuite Area: The testsuite used to check the correctness of rustc C-cleanup Category: PRs that clean code up or issues documenting cleanup. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.50.0
Development

Successfully merging this pull request may close these issues.
5 participants
@poliorcetics @jyn514 @bors @rust-highfive @rustbot