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

Remove unused code redux #594

Merged
merged 3 commits into from
Feb 8, 2020
Merged

Remove unused code redux #594

merged 3 commits into from
Feb 8, 2020

Conversation

jyn514
Copy link
Member

@jyn514 jyn514 commented Feb 6, 2020 •
edited
Loading

This fixes the panic from before, I added a test and also built a couple crates locally.

I also refactored copy_doc_dir to remove the gratuitous use of PathBuf.

r? @koenaad

jyn514 and others added 3 commits February 6, 2020 17:40
@jyn514
Remove unused code (rust-lang#558)
6eda116 
* remove copy_dir

* make utils pub(crate) where possible

* remove dead code in rustc version

* make more things pub(crate)

* only compile ApplyMode::Temporary in test mode

* remove unused argument

* remove unused `handle_html` argument

* don't export metadata

* remove `pub` where possible

* remove unused import

* make create_pool pub(crate)

* fix bad merge

* pub(crate) doesn't make sense in lib.rs

Co-Authored-By: Koenraad Verheyden <[email protected]>

* address review comments re: copy test

* remote system dependencies from metadata

this was causing confusion, see rust-lang#164

* re-comment failing test

* fix failing test

Co-authored-by: Koenraad Verheyden <[email protected]>
@jyn514
Fix bug in copy_doc_dir
eabe8a3 
Before, it would append `doc/` on every recursive call.
This moves the responsibility of adding `doc/` to the caller.
@jyn514
refactor copy_doc_dir to use less allocation
e55ae87
@yvrhdn
Copy link

yvrhdn commented Feb 7, 2020

The last commits looks good! Great job on tracking down the doc/ recursion.

When I test it locally the styling is all messed up, I'm not sure if this is related to this PR or that my local setup broke :(

Screenshot 2020-02-08 at 00 03 43

In the console I get a bunch of errors when loading resources that contain -20200206-1.43.0-nightly-442ae7f04:

Loading failed for the <script> with source “http://localhost:3000/storage-20200206-1.43.0-nightly-442ae7f04.js”.
This page uses the non standard property “zoom”. Consider using calc() in the relevant property values, or using “transform” along with “transform-origin: 0 0”.
Loading failed for the <script> with source “http://localhost:3000/theme-20200206-1.43.0-nightly-442ae7f04.js”.
Loading failed for the <script> with source “http://localhost:3000/main-20200206-1.43.0-nightly-442ae7f04.js”.
ReferenceError: addSearchOptions is not defined

@yvrhdn
Copy link

yvrhdn commented Feb 7, 2020 •
edited
Loading

Nevermind the above ☝️, this also happens if I build/run from the master branch...

So... this PR seems good but I can't reliably test it now... And you might want to ask for a second opinion after the trouble the last PR I reviewed caused on production 🙈

@yvrhdn yvrhdn mentioned this pull request Feb 7, 2020
Merged
@jyn514
Copy link
Member Author

jyn514 commented Feb 8, 2020

So... this PR seems good but I can't reliably test it now... And you might want to ask for a second opinion after the trouble the last PR I reviewed caused on production

It's ok, this has happened before even with me and pietro reviewing each others code ... We need test cases for builds (also, it would be nice to get #570 merged).

When I test it locally the styling is all messed up, I'm not sure if this is related to this PR or that my local setup broke :(

As you've noticed, this is not because of this PR. I'm confident this won't change any of the CSS (how could it?). The way I tested locally was by building crates (docker-compose run web build crate regex 1.3.0).

@yvrhdn
Copy link

yvrhdn commented Feb 8, 2020

As you've noticed, this is not because of this PR. I'm confident this won't change any of the CSS (how could it?). The way I tested locally was by building crates (docker-compose run web build crate regex 1.3.0).

Yeah, you are right. Just wanted to make sure this wasn't caused by an incorrect file copy.

yvrhdn
yvrhdn approved these changes Feb 8, 2020
View reviewed changes
Copy link

@yvrhdn yvrhdn left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@jyn514 jyn514 merged commit 42d6922 into rust-lang:master Feb 8, 2020
@jyn514 jyn514 deleted the refactor-redux branch February 8, 2020 18:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yvrhdn yvrhdn yvrhdn approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jyn514 @yvrhdn