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

Website issue tracker link and better search performance #6483

Merged
merged 2 commits into from
Dec 23, 2020
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 31 additions & 23 deletions util/gh-pages/index.html
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ <h1>ALL the Clippy Lints</h1>
<div class="col-md-12 form-horizontal">
<div class="input-group">
<label class="input-group-addon" id="filter-label" for="filter-input">Filter:</label>
<input type="text" class="form-control" placeholder="Keywords or search string" id="filter-input" ng-model="search" />
<input type="text" class="form-control" placeholder="Keywords or search string" id="filter-input" ng-model="search" ng-model-options="{debounce: 50}"/>
<span class="input-group-btn">
<button class="btn btn-default" type="button" ng-click="search = ''">
Clear
Expand Down Expand Up @@ -119,6 +119,7 @@ <h4 class="list-group-item-heading">
{{title}}
</h4>
<div class="list-group-item-text" ng-bind-html="text | markdown"></div>
<a ng-if="title == 'Known problems'" href="https://github.com/rust-lang/rust-clippy/issues?q=is%3Aissue+is%3Aopen+{{lint.id}}">Search on GitHub</a>
</li>
</ul>
</article>
Expand Down Expand Up @@ -180,6 +181,22 @@ <h4 class="list-group-item-heading">
}
}

function searchLint(lint, therm) {
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for pulling this out. As an aside, I think therm should be term here and below?

Copy link
Member Author

Choose a reason for hiding this comment

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

Spelling is not always my strong suite 😅 thanks for pointing that out. It should be fixed now 🙃

for (const field in lint.docs) {
// Continue if it's not a property
if (!lint.docs.hasOwnProperty(field)) {
continue;
}

// Return if not found
if (lint.docs[field].toLowerCase().indexOf(therm) !== -1) {
return true;
}
}

return false;
}

angular.module("clippy", [])
.filter('markdown', function ($sce) {
return function (text) {
Expand Down Expand Up @@ -216,40 +233,31 @@ <h4 class="list-group-item-heading">
};

$scope.bySearch = function (lint, index, array) {
let search_str = $scope.search;
let searchStr = $scope.search;
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for js-ifying this!

// It can be `null` I haven't missed this value
if (search_str == null || search_str.length == 0) {
if (searchStr == null || searchStr.length < 3) {
return true;
}
search_str = search_str.toLowerCase();
searchStr = searchStr.toLowerCase();

// Search by id
let id_search = search_str.trim().replace(/(\-| )/g, "_");
if (lint.id.includes(id_search)) {
if (lint.id.indexOf(searchStr.replace("-", "_")) !== -1) {
Copy link
Member

Choose a reason for hiding this comment

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

The previous code also replaced spaces with underscores. Not sure if this is still desired but I at least wanted to point it out :)

Copy link
Member Author

@xFrednet xFrednet Dec 22, 2020

Choose a reason for hiding this comment

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

Jup this is on purpose. The space search is now covered by line 252 if (lint.id.indexOf(therms[index]) !== -1) this allows the words to be out of order which is likely when somebody just types them in. It could also reduce performance since we are not using a regex in the replace function, but I'm not sure about that :)

Copy link
Member

Choose a reason for hiding this comment

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

Ah of course, that makes sense!

(I would expect any decent JS enngine to optimize the simple regex to a byte match so it's probably as fast as the literal replace.)

return true;
}

// Search the description
// The use of `for`-loops instead of `foreach` enables us to return early
let search_lint = (lint, therm) => {
for (const field in lint.docs) {
// Continue if it's not a property
if (!lint.docs.hasOwnProperty(field)) {
continue;
}

// Return if not found
if (lint.docs[field].toLowerCase().includes(therm)) {
return true;
}
}
return false;
};
let therms = search_str.split(" ");
let therms = searchStr.split(" ");
for (index = 0; index < therms.length; index++) {
if (!search_lint(lint, therms[index])) {
return false;
if (lint.id.indexOf(therms[index]) !== -1) {
continue;
}

if (searchLint(lint, therms[index])) {
continue;
}

return false;
}

return true;
Expand Down