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

Clean up rustdoc js testers #70224

Merged
merged 3 commits into from
Apr 3, 2020
Merged

Clean up rustdoc js testers #70224

merged 3 commits into from
Apr 3, 2020

Conversation

GuillaumeGomez
Copy link
Member

I realized after the improvement made by @ollie27 on the rustdoc-js-tester that a lot of code was actually duplicated. This PR intends to remove this duplication, making it simpler to update in case of future main.js updates.

r? @ollie27

cc @kinnison

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 21, 2020
ollie27
ollie27 reviewed Mar 21, 2020
View reviewed changes
Copy link
Member

@ollie27 ollie27 left a comment

Choose a reason for hiding this comment

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

Could we combine these three files into a single tester.js file that works for both test suites?

src/tools/rustdoc-js-std/tester.js
return null;
}
// We make a reverse sort to have the "highest" file. Very useful in case you didn't clean up
// you std doc folder...
Copy link
Member

Choose a reason for hiding this comment

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

This is a lot of code for such an edge case.

Instead, how about passing the resource suffix as an argument to tester.js and using that to build the filenames? That way we won't need the findFile or readFileMatching functions at all and there won't be any issues with outdated files.

Copy link
Member Author

Choose a reason for hiding this comment

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

That'd make things simpler indeed!

@GuillaumeGomez
Copy link
Member Author

@ollie27 I'm a bit shared on this: it'd mean adding more options, which I'm not a big fan of.

@ollie27
Copy link
Member

ollie27 commented Mar 24, 2020

it'd mean adding more options, which I'm not a big fan of.

I don't think merging the files would, but either way it can done in a follow up PR.

@joelpalmer joelpalmer added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 2, 2020
@Dylan-DPC-zz Dylan-DPC-zz 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 Apr 2, 2020
@Dylan-DPC-zz
Copy link

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Apr 2, 2020

📌 Commit c88be65 has been approved by Dylan-DPC

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 2, 2020
@GuillaumeGomez
Copy link
Member Author

Ok, opening an issue to not lose track of it then. Thanks a lot @ollie27 !

@GuillaumeGomez GuillaumeGomez mentioned this pull request Apr 2, 2020
Closed
@ollie27
Copy link
Member

ollie27 commented Apr 2, 2020

#70224 (comment) still hasn't been resolved. I guess I should have been clearer but that was the reason I hadn't r+'d this.

@Centril Centril mentioned this pull request Apr 3, 2020
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 3, 2020
@bors
Auto merge of rust-lang#70726 - Centril:rollup-zrdkkpt, r=Centril
548afdb 
Rollup of 5 pull requests

Successful merges:

 - rust-lang#68334 (AArch64 bare-metal targets: Build rust-std)
 - rust-lang#70224 (Clean up rustdoc js testers)
 - rust-lang#70532 (Miri engine: stronger type-based sanity check for assignments)
 - rust-lang#70698 (bootstrap: add `--json-output` for rust-analyzer)
 - rust-lang#70715 (Fix typo in operands section)

Failed merges:

r? @ghost
@bors bors merged commit a80b491 into rust-lang:master Apr 3, 2020
@GuillaumeGomez GuillaumeGomez deleted the clean-up-rustdoc-js-testers branch April 3, 2020 12:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ollie27 ollie27 ollie27 left review comments

Assignees

@ollie27 ollie27

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@GuillaumeGomez @ollie27 @Dylan-DPC-zz @bors @rust-highfive @joelpalmer