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

Display search results in order returned from apm #1042

Merged
merged 5 commits into from Jan 14, 2019

Conversation

Projects
None yet
4 participants
@50Wliu
Copy link
Member

50Wliu commented Jan 19, 2018

Requirements

  • Filling out the template is required. Any pull request that does not include enough information to be reviewed in a timely manner may be closed at the maintainers' discretion.
  • All new code requires tests to ensure against regressions

Description of the Change

This PR removes the custom sorting logic that the setting-view performs after getting search results from apm. My reasoning is that with the recent changes to apm's sorting we should no longer need to add extra sorting in settings-view. This also means that the search results will be consistent regardless of whether the search was executed through settings-view, apm, or https://atom.io/packages/search.

Alternate Designs

None.

Benefits

Removal of code that had minimum benefits as well as consistency with other search methods.

Possible Drawbacks

It is possible that some people might prefer the custom sorting settings-view did.

Applicable Issues

None.

/cc @Ingramz do you have any opinions about this?

@50Wliu 50Wliu force-pushed the wl-use-apm-results branch from 73f62d4 to 46f187b Jan 19, 2018

@50Wliu 50Wliu force-pushed the wl-use-apm-results branch from 46f187b to 38b9408 Jan 19, 2018

Object.assign metadata, {readme, downloads, stargazers_count}
.sort (a, b) -> b.downloads - a.downloads
body.filter (pack) ->
pack.releases?.latest?.map ({readme, metadata, downloads, stargazers_count}) ->

This comment has been minimized.

@Ingramz

Ingramz Jan 19, 2018

Contributor

.filter and .map need to be on the same level: first filter is done, then map, not one inside other.

This comment has been minimized.

@50Wliu

50Wliu Jan 19, 2018

Author Member

Ah, I must have touched that line by accident when removing .sort. Then Coffeelint gave me errors and I guess I didn't notice and "fixed" it incorrectly :/.

Of course, this would have been avoided with JS 😀.

@Ingramz

This comment has been minimized.

Copy link
Contributor

Ingramz commented Jan 20, 2018

I don't mind if search worked identically everywhere (website, apm, and settings-view). I think it makes the ecosystem more consistent and predictable.

While the new ordering done serverside surfaces relevant packages without requiring assitance from the clientside (searching for blade shows nicely all blade related packages on top, which I like), it feels a little off. Maybe it is because a more popular package is not a top hit, but it's not as big of a deal as all 3 results are very close to the top.

Searching for common terms like php or javascript also do seem to have a clear boundary from which packages containing the search term are promoted to the top, however the ordering among them seems off. If the recent serverside improvement is essentially replicating the addCloseMatches and highlightExactMatch done in settings-view, then I would suggest sort by downloads done on the results that qualify for the addCloseMatches criteria.

Settings view does not implement pagination for search results, which means that it is important that good search results make it to the first page, preferably above the fold. This might become more important if all relevant search results do not fit into the first page.

In general I like where this is going and I am interested in what others think.

@Ben3eeE

This comment has been minimized.

Copy link
Member

Ben3eeE commented Jan 26, 2018

@daviwil

This comment has been minimized.

Copy link
Member

daviwil commented Jan 14, 2019

Thanks @50Wliu! I agree that it's better to respect atom.io's search result ordering in Atom and focus future refinements in that codebase. Merging it!

@daviwil daviwil merged commit 3b86c07 into master Jan 14, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@daviwil daviwil deleted the wl-use-apm-results branch Jan 14, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.