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

Handle correctly similar results from multiple projects #383

Merged
merged 3 commits into from May 2, 2019

Conversation

Projects
None yet
2 participants
@rafeca
Copy link
Contributor

commented May 1, 2019

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 fixes #382 by adding any root that contains a filter result to the list of final results.

Alternate Designs

We could change fuzzy-native to accept an object that contains both the full path of the result (or an id) + the string to use for the matcher (as the standard fuzzy matcher does), but that would require sending a much bigger object to the native side which could slowdown thing (plus the change would be bigger and affect other repos).

Benefits

Fixes #382

Possible Drawbacks

None that I can think of.

Applicable Issues

#382

@rafeca rafeca requested a review from nathansobo May 1, 2019

@rafeca rafeca force-pushed the fix-issue-with-multiple-roots branch from 672144e to c08e006 May 1, 2019

@rafeca rafeca force-pushed the fix-issue-with-multiple-roots branch from c08e006 to 9d23d9a May 1, 2019

@nathansobo
Copy link
Contributor

left a comment

I requested a few minor changes, but also noticed a pretty big issue that already existed prior to these changes, which is our use of fs.existsSync on the main thread. Maybe we address that one in a follow-up PR?


if (fs.existsSync(absolutePath)) {
return absolutePath
if (fs.existsSync(absolutePath)) {

This comment has been minimized.

Copy link
@nathansobo

nathansobo May 1, 2019

Contributor

I realize that we this existsSync call wasn't introduced with this change, but I really think we should avoid performing synchronous I/O on the main thread. You never know how long the disk read is going to block for, especially on a network drive. One way to do this would be to retain the absolute path of the results returned from fuzzy-native. I'm not familiar with its current API, so this may require changes to that module. I doubt they would be too difficult and I am happy to help out with it.

This comment has been minimized.

Copy link
@rafeca

rafeca May 1, 2019

Author Contributor

Good point! I briefly looked at using the async version but that would require changes to atom-select-list since it currently expects the filterFn function to be sync, and changing that was not trivial.

I think that what you mention is similar to the alternate design on the description: that can definitely be done and in general terms it would be a better solution, we just need to make sure that the additional payload that we're serializing in order to pass it to the native module does not regress the overall filtering performance.

This comment has been minimized.

Copy link
@nathansobo

nathansobo May 1, 2019

Contributor

Oh, I totally missed your comments in the alternate design section. Sorry about that. I think it's worth pursuing that route fairly soon.

@@ -390,27 +390,32 @@ module.exports = class FuzzyFinderView {
return {uri: filePath, filePath, label}
}

getAbsolutePath (relativePath) {
const directories = atom.project.getDirectories()
parseResultsFromFuzzyNative (results) {

This comment has been minimized.

Copy link
@nathansobo

nathansobo May 1, 2019

Contributor

This is minor, but I think "parse" isn't really the right word here. Maybe convertFuzzyNativeResultsToViews or something along those lines?

@@ -351,6 +360,44 @@ describe('FuzzyFinder', () => {

expect(Array.from(projectView.element.querySelectorAll('li')).filter(a => a.textContent.includes('child-file.txt')).length).toBe(1)
})

it('Return all the results if they have the same relative path across multiple root folders', async () => {

This comment has been minimized.

Copy link
@nathansobo

nathansobo May 1, 2019

Contributor

I generally prefer test descriptions to read like an english sentence when appended to "it":

it("returns all the results if..."

@rafeca rafeca requested a review from nathansobo May 2, 2019

@nathansobo nathansobo merged commit 9b60cc7 into master May 2, 2019

2 checks passed

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

@nathansobo nathansobo deleted the fix-issue-with-multiple-roots branch May 2, 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.