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 #23

Open
wants to merge 12 commits into
base: gh-pages
Choose a base branch
from
Open

search #23

wants to merge 12 commits into from

Conversation

kedashoe
Copy link
Member

@kedashoe kedashoe commented Jan 30, 2017

Supersedes #11. Now using hindley milner notation for the search input. Things seem to be working pretty well?

I've brought in browserify to tie in external packages and uglify to minify. I don't really know anything about minification, so if anyone has some thoughts there I am happy to go with something else for that step.

One kinda funny thing: you can't search for combinators by name because the input parser does not allow function names (correctly, I believe?) to start with upper case letters. We could probably create an exception for them.

Todo:

  • lint src/search.js
  • properly index type variables (so that eg x -> x matches a -> a)
  • fuzzy matching too many results?
  • update query string at /search/ as input changes
  • parse query string to populate search field
  • add search field to / which redirects to /search/?q=…

@kedashoe kedashoe mentioned this pull request Jan 30, 2017
@davidchambers
Copy link
Member

Very cool, Kevin!

I'm sure I'll have more thoughts, but these are my initial ones:

  • When searching, I expect the list of results to contain only matches and their ancestors. "Overview", for example, need never appear in the list of results.

  • Either a b produces many matches, but Either x y produces none. This suggest that we're not treating type variables as variables. a -> a and x -> x should both match S.I.

  • Maybe#map produces many matches for some reason.

@kedashoe
Copy link
Member Author

When searching, I expect the list of results to contain only matches and their ancestors. "Overview", for example, need never appear in the list of results.

makes sense

Either a b produces many matches, but Either x y produces none. This suggest that we're not treating type variables as variables. a -> a and x -> x should both match S.I

ya, should have mentioned this one

Maybe#map produces many matches for some reason.

I think I am letting the fuzzy searcher be too generous? Will play around with it.

@davidchambers
Copy link
Member

Having given the matter more thought I now believe the search functionality should have its own URL:

https://sanctuary.js.org/search/

We could then share links to search results which would be useful. For example:

https://sanctuary.js.org/search/?q=List%20a%20-%3E%20Maybe%20a

We could make this discoverable by including a text input near the top of the main page (as you have currently), and setting window.location to the appropriate value when a query is submitted.

@kedashoe
Copy link
Member Author

Pushed an update. The search now manages its own list rather than showing/hiding items in the ToC (didn't occur to me at first, but this was always going to be necessary). x -> Maybe y should now return appropriate results. Do you want the URL stuff to be part of this PR?

@davidchambers
Copy link
Member

Pushed an update.

Looks good!

x -> Maybe y should now return appropriate results.

I see these results:

  • Maybe#ap :: Maybe (a -> b) ~> Maybe a -> Maybe b
  • Maybe#chain :: Maybe a ~> (a -> Maybe b) -> Maybe b
  • Maybe#equals :: Maybe a ~> b -> Boolean
  • Maybe#of :: Maybe a ~> b -> Maybe b
  • Maybe#reduce :: Maybe a ~> ((b, a) -> b) -> b -> b
  • maybe :: b -> (a -> b) -> Maybe a -> b
  • encase :: (a -> b) -> a -> Maybe b
  • maybeToEither :: a -> Maybe b -> Either a b
  • eitherToMaybe :: Either a b -> Maybe b

The results in bold seem out of place to me.

Do you want the URL stuff to be part of this PR?

Yes please! Let me know if I can help with this in any way. :)

@kedashoe
Copy link
Member Author

ah, forgot to mention that. In order to allow us to find functions and methods, I am compiling methods to their function equivalent. So

:: Maybe a ~> b -> Boolean

is compiled to

:: t0 -> Maybe t1 -> Boolean

https://github.com/kedashoe/hindley-milner-search/blob/master/src/hm-search.js#L68-L78

@davidchambers
Copy link
Member

I am compiling methods to their function equivalent

It might be nice for ~> to return all methods, and for Maybe a ~> to return all Maybe methods. It's worth reconsidering your rewrite rule.

By the way, I'm quite confused by the results that ~> currently returns!

@kedashoe
Copy link
Member Author

It might be nice for ~> to return all methods, and for Maybe a ~> to return all Maybe methods. It's worth reconsidering your rewrite rule.

I had thought we were trying to discourage the use of methods? The difficulty is that I think we definitely want Maybe a -> b to return methods if they match, but it would not be straightforward to be able to match both function syntax and the Maybe a ~> b syntax (off the top of my head, every signature would need to have multiple parse trees which would all need to be searched).

By the way, I'm quite confused by the results that ~> currently returns!

Anything that we cannot parse we treat as a name, and ~> doesn't parse to anything, so it falls back to a name. Those signatures that it matches are also ones we cannot (currently) parse, so they also become names, and the fuzzy matcher likes those best. The fuzzy matcher returns a score, maybe if everything scores very poorly I should say "No matches"?

@davidchambers
Copy link
Member

This discussion has made it apparent to me that fuzzy matching algorithms don't sit well with me. Web search engines such as Google are incredibly useful, of course, and are necessarily fuzzy. In more limited domains, though, I prefer algorithms which I understand (or could understand given enough time). I would like to be able to state definitively—without consulting a computer—the matches for a given search term.

I'm quite possibly anomalous.

I don't want my preferences to block this useful feature. Let's get the user interface in good shape then release the feature so that people can start using it and provide feedback. We can tweak the weightings as we see fit.

@kedashoe
Copy link
Member Author

I don't have any attachment to the fuzzy search. I was planning to write a simpler one at some point anyway. For now, I'll play around with the scores it returns, I think returning some results no matter what is causing more harm than good.

@kedashoe
Copy link
Member Author

I've updated the fuzzy searcher. No third party lib, just very simple search and score. Results look pretty good to me. I've also removed type info from sigs that can't be parsed, this seemed to be causing some confusion.

Also rebased. The bold that was added looks really nice in the search results :)

When you are ready to merge, please post to let me know before merging. I will release 1.0.0 of both the parser and searcher at that point, update the deps, make sure everything is still working, and then let you know it is ready to merge.

package.json Outdated
"marked": "0.3.5",
"sanctuary-style": "0.4.x"
"sanctuary-style": "0.4.x",
"uglify-js": "^2.7.5"
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason to use ^2.7.5 rather than 2.7.x in this case?

src/search.js Outdated
return function(el) {
container.appendChild(el);
};
}
Copy link
Member

Choose a reason for hiding this comment

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

Since we have Sanctuary available to us, should we use S.curry2 here?

src/search.js Outdated
var codes = document.getElementById('toc').querySelectorAll('li a code');
var as = map(getParent, codes);
var newEls = map(initSigResult, as);
return newEls;
Copy link
Member

Choose a reason for hiding this comment

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

I don't think the names add clarity here. Let's nest expressions:

return map(initSigResult, map(getParent, codes));

Copy link
Member

Choose a reason for hiding this comment

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

Do we have compose so we don't map over it twice:

map(compose(initSigResult, getParent), codes)

src/search.js Outdated

// Node -> String
function extractSignatureText(el) {
return el.innerText.trim().replace(/\s+/g, ' ');
Copy link
Member

Choose a reason for hiding this comment

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

S.unwords(S.words(el.innerText)) would be nice here. :)

src/search.js Outdated
}
else {
hide(state.containerNo);
}
Copy link
Member

Choose a reason for hiding this comment

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

I don't much like the show and hide functions as they're impure. I'd prefer this mutation to appear in situ:

state.containerNo.style.display =
  matches.length === 0 && q.length > 0 ? 'block' : 'none';

src/search.js Outdated
clearChildren(state.container);
// if there is no query, do not show any matches
if (q.length > 0) {
for (var i = 0; i < matches.length; ++i) {
Copy link
Member

Choose a reason for hiding this comment

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

Let's use matches.forEach here to avoid the index variable.

src/search.js Outdated
searchInput.focus();
searchInput.addEventListener('keyup', handleSearchKeyUp);
});

Copy link
Member

Choose a reason for hiding this comment

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

👾

style.css Outdated
@@ -111,6 +111,10 @@ a code {
top: -0.05em;
}

#search-results > div {
Copy link
Member

Choose a reason for hiding this comment

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

Matching elements by tag name is inadvisable. Let's add an id or class name to the div and use a more precise selector here.

src/search.js Outdated
return Array.prototype.map.call(x, f);
}

// Node -> Node
Copy link
Member

Choose a reason for hiding this comment

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

I'd like to be able to run ack '^ *//' src/search.js to view all the type signatures. Please update this signature (and others) to include the function name:

//  getGrandparent :: Node -> Node

@kedashoe
Copy link
Member Author

updated.

My eslint knowledge is next to zero. I've fixed all the ones that looked like they should be fixed and am left with:

  • error 'require' is not defined
  • error Use the function form of 'use strict'
  • error Expected a function declaration

On that last one, since I am using S now and currying a few functions, I changed all function declarations to be var foo = function foo... to be consistent.

@davidchambers
Copy link
Member

Are you happy for me to push to your branch, Kevin? I'd like to have a go at adding a dedicated /search/ page.

@kedashoe
Copy link
Member Author

kedashoe commented Mar 7, 2017

Sure, do I need to do anything?

@davidchambers
Copy link
Member

Sure, do I need to do anything?

Nope. I'm going to rebase to resolve merge conflicts before I make any changes. Once I've done so you'll need to check out a fresh copy of this branch to avoid merge conflicts locally.

@davidchambers
Copy link
Member

We must resolve kedashoe/hindley-milner-search#4 before merging this pull request.

@davidchambers
Copy link
Member

Do we have any control over the fuzziness? It's great that a search for map finds map, bimap, and promap, but I don't think it should find Maybe#inspect.

@kedashoe
Copy link
Member Author

Do we have any control over the fuzziness? It's great that a search for map finds map, bimap, and promap, but I don't think it should find Maybe#inspect.

see kedashoe/hindley-milner-search#6. will merge and release if that looks good to you.

@kedashoe
Copy link
Member Author

will merge and release if that looks good to you.

0.2.0 up

@davidchambers
Copy link
Member

I've updated this branch to [email protected]. We're now specifying {fuzzy: false} which fixed the overeager matching of names, but now a search for a -> a returns no results.

@kedashoe, can we perform substring matching of function names without losing the ability to search by type?

@kedashoe
Copy link
Member Author

Sorry about that, I've released v0.2.1 with the fix.

@davidchambers
Copy link
Member

Thanks for the update, Kevin. :)

I still can't find S.I: neither I nor a -> a finds it. Have you any idea why this is the case?

src/search.js Outdated
var search = HMS.init(Array.prototype.map.call($els, function(el) {
return el.innerText.trim().split(/\s+/).join(' ');
}), {fuzzy: false}).search;
var $prevSearch = '';
Copy link
Member Author

Choose a reason for hiding this comment

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

What does $ prefix mean? Why does $prevSearch have one but not q below?

Copy link
Member

Choose a reason for hiding this comment

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

In 6e24452 I replaced the state object with various variables. I assumed each of these variables to be stateful, and used the $ prefix to remind myself of this.

src/search.js Outdated
$container.removeChild($container.childNodes[0]);
}
// if there is no query, do not show any matches
if (q !== '') {
Copy link
Member Author

Choose a reason for hiding this comment

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

I originally had the empty string return all matches simply because I was culling results from the current DOM rather than building a new list. I'm not sure which would be more common, but I can add an option to return all or none on empty input and we wouldn't need this conditional in userland either way.

Copy link
Member

Choose a reason for hiding this comment

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

Since we'll be displaying results at /search/ rather than at /, displaying all results for an empty query seems right to me. I've removed this check. :)

Copy link
Member Author

Choose a reason for hiding this comment

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

I had to move this logic a little in the latest release. Not a million percent sure we're on the same page but just let me know if empty string is not doing what you want now.

@kedashoe
Copy link
Member Author

New release up, a -> a and I should both find I now.

One other note, when I was playing around with my local copy of sanctuary-site, I was not getting many results for anything at first. This was due to \u2060 in our arrows (I feel like I remember discussion about this but couldn't find the PR), removing those fixed things.

Makefile Outdated
@@ -22,6 +23,12 @@ vendor/ramda.js: node_modules/ramda/dist/ramda.js
vendor/%.js: node_modules/%/index.js
cp '$<' '$@'

js/hm-search.js: node_modules/hindley-milner-search/hm-search.js Makefile
$(BROWSERIFY) --standalone HMS -- '$<' >'$@'
Copy link
Member

Choose a reason for hiding this comment

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

It would be nice if a browser build were available. This is related to kedashoe/hindley-milner-parser-js#1.

Copy link
Member Author

Choose a reason for hiding this comment

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

browser standalone files included in v0.2.3. You should see hms.js and hms.min.js now. Exported as HMS as you did.

Copy link
Member

Choose a reason for hiding this comment

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

Excellent! I've updated the makefile to simply copy hms.js, so we no longer depend on Browserify. :)

@chughes87
Copy link

Bump on this PR! I'd love to see this merged soon...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants