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

search.js: split out nested functions #91992

Closed
wants to merge 1 commit into from

Conversation

jsha
Copy link
Contributor

@jsha jsha commented Dec 16, 2021

execSearch was a giant function with a number of other very large functions embedded inside it. This moves the embedded functions out of execSearch, so execSearch's function signature can be read next to its code.

sortResults was relying on capturing certain variables from its environment, so I made those into explicity parameters passed into sortResults.

This doesn't change any functionality. It also doesn't change any comments or parameters other than those used by sortResults. It just moves lines from one place to another and changes indentation.

Discussed on Discord.

r? @GuillaumeGomez

`execSearch` was a giant function with a number of other very large
functions embedded inside it. This moves the embedded functions out of
execSearch, so execSearch's function signature can be read next to its
code.

`sortResults` was relying on capturing certain variables from its
environment, so I made those into explicity parameters passed into
`sortResults`.
@jsha jsha added T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. A-rustdoc-search Area: Rustdoc's search feature labels Dec 16, 2021
@rust-highfive
Copy link
Collaborator

Some changes occurred in HTML/CSS/JS.

cc @GuillaumeGomez

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 16, 2021
@rust-log-analyzer
Copy link
Collaborator

The job x86_64-gnu-llvm-12 failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
Set({"src/tools/lint-docs"}) not skipped for "bootstrap::test::LintDocs" -- not in ["src/tools/tidy"]
 finished in 1.221 seconds
Generating lint docs (x86_64-unknown-linux-gnu)
Suite("src/test/rustdoc-js-std") not skipped for "bootstrap::test::RustdocJSStd" -- not in ["src/tools/tidy"]
tmp.js:2
function hasOwnPropertyRustdoc(obj,property){return Object.prototype.hasOwnProperty.call(obj,property)}exports.hasOwnPropertyRustdoc = hasOwnPropertyRustdoc;function onEach(arr,func,reversed){if(arr&&arr.length>0&&func){var length=arr.length;var i;if(reversed){for(i=length-1;i>=0;--i){if(func(arr[i])){return true}}}else{for(i=0;i<length;++i){if(func(arr[i])){return true}}}}return false}exports.onEach = onEach;var itemTypes=["mod","externcrate","import","struct","enum","fn","type","static","trait","impl","tymethod","method","structfield","variant","macro","primitive","associatedtype","constant","associatedconstant","union","foreigntype","keyword","existential","attr","derive","traitalias",];exports.itemTypes = itemTypes;var MAX_LEV_DISTANCE=3;exports.MAX_LEV_DISTANCE = MAX_LEV_DISTANCE;var MAX_RESULTS=200;exports.MAX_RESULTS = MAX_RESULTS;var NO_TYPE_FILTER=-1;exports.NO_TYPE_FILTER = NO_TYPE_FILTER;var GENERICS_DATA=2;exports.GENERICS_DATA = GENERICS_DATA;var NAME=0;exports.NAME = NAME;var INPUTS_D

ReferenceError: tokenizeQuery is not defined
    at Object.execSearch (tmp.js:2:11499)
    at runSearch (/checkout/src/tools/rustdoc-js/tester.js:294:26)
    at runChecks (/checkout/src/tools/rustdoc-js/tester.js:386:22)
    at checkFile (/checkout/src/tools/rustdoc-js/tester.js:463:12)
    at /checkout/src/tools/rustdoc-js/tester.js:487:23
    at Array.forEach (<anonymous>)
    at main (/checkout/src/tools/rustdoc-js/tester.js:483:45)
    at Object.<anonymous> (/checkout/src/tools/rustdoc-js/tester.js:493:14)
    at Module._compile (internal/modules/cjs/loader.js:778:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:789:10)


command did not execute successfully: "/usr/bin/node" "/checkout/src/tools/rustdoc-js/tester.js" "--crate-name" "std" "--resource-suffix" "1.59.0" "--doc-folder" "/checkout/obj/build/x86_64-unknown-linux-gnu/doc" "--test-folder" "/checkout/src/test/rustdoc-js-std"


Build completed unsuccessfully in 0:34:30

@jsha
Copy link
Contributor Author

jsha commented Dec 16, 2021

It looks like this runs into a problem with src/tools/rustdoc-js/tester.js. The tester doesn't actually load search.js as a module; instead it processes search.js to try and find certain functions and variables and extract them. So it's extracting execQuery but not the functions execQuery depends on.

I think the fix may be to just load search.js as a module.

@GuillaumeGomez
Copy link
Member

It's actually complicated to import search as is because of the global variables we use. The simplest solution here would be to just list the functions you extract in the JS tester and that's it.

Also, can you wait for #91977 to be merged first please? :)

@jsha
Copy link
Contributor Author

jsha commented Dec 16, 2021

Yes, definitely happy to wait for #91977, just wanted to get this idea up for a look.

It's actually complicated to import search as is because of the global variables we use.

Yes, this is true, but I think it's worthwhile to refactor somewhat so it doesn't need global variables. Then we can have a pure function that takes as input the corpus, a query, and some config options. That should make it simpler to test and we can remove a lot of the tester code. That will also open up the possibility of using JS code coverage tools to see what needs more testing.

@bors
Copy link
Contributor

bors commented Dec 18, 2021

☔ The latest upstream changes (presumably #92064) made this pull request unmergeable. Please resolve the merge conflicts.

@jsha
Copy link
Contributor Author

jsha commented Dec 30, 2021

I'm going to close this for now since it would generate a lot of conflicts with #90630, and this is a fairly straightforward refactoring that can be applied after that PR lands.

@jsha jsha closed this Dec 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-rustdoc-search Area: Rustdoc's search feature S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants