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

First attempt with DocQuery #24

Merged
merged 1 commit into from
Aug 29, 2015

Conversation

seongjaelee
Copy link
Owner

Hi, could we put this change onto atom notation velocity?

seongjaelee added a commit that referenced this pull request Aug 29, 2015
@seongjaelee seongjaelee merged commit ae435d7 into seongjaelee:master Aug 29, 2015
@jonmagic
Copy link
Contributor

I'm not sure it was quite ready? It's been a while since I worked on it but as far as I remember it needed some work around the autocomplete text selection part.

I can try and return to this this weekend.

@jonmagic
Copy link
Contributor

So I did check and I've been using my docquery version for months now. It is buggy though, here's an example:

@seongjaelee
Copy link
Owner Author

Thanks very much for your reply. My test failure was caused by my dev env setup.
I found out there is another branch going on using chokidar.
#23

I need to learn pros and cons between two branches and decide which one to use first.

@deltaidea
Copy link

@seongjaelee, use this PR instead of #23.

chokidar package is a replacement for pathwatcher - it's a tool to watch for changes in a directory or a file.

This PR, on the other hand, completely reimplements the whole core of note handling. It can also potentially at least partially fix performance issues like #22 and deltaidea/notational-windows#1.

Edit: The #23 will probably go towards DocQuery if there will be the same issue on Windows.
Edit 2: DocQuery already uses chokidar.

@jonmagic
Copy link
Contributor

It can also potentially at least partially fix performance issues like #22 and deltaidea/notational-windows#1.

It won't do this part "yet". Serializing the search index and saving it to disk and then reloading it will need to be implemented still (olivernn/lunr.js#60 (comment) has more details on how this might work).

@deltaidea
Copy link

There're two different issues there:

  • Initial indexing takes time
  • Searching through an already built index takes time

The second issue surely can be solved by this PR alone.

@jonmagic
Copy link
Contributor

The second issue surely can be solved by this PR alone.

Ahh yes, very good point 👍

@deltaidea
Copy link

I have a person who has just installed the powered-by-docquery branch and apparently confirms that the searching through a large index issue is indeed fixed. Thanks, @jonmagic!

👍 for this PR.

Edit: this PR solves performance issues when you have 10 files with 10,000 characters each (deltaidea/notational-windows#1), but probably not when you have 10,000 files with 10 characters each (#22).

@seongjaelee
Copy link
Owner Author

I have been trying to push powered-by-docquery branch, but I have been facing test failures:
https://travis-ci.org/seongjaelee/notational-velocity/builds/78524556

It seems like fsevents is related to this... I tried to resolve this issue for two full days, but couldn't resolve it. I thought it was my dev env problem (osx), but it shows the same error on Travis (linux). Do you guys have any idea?

@deltaidea
Copy link

I'll probably try to just rewrite the tests. Luckily, there aren't too many of them. I have a suspicion that the tests themselves cause problems.

@seongjaelee
Copy link
Owner Author

The docquery branch, which is the direct copy of powered-by-docquery branch, only has one test - toggling on the package and check if we really have to toggled window. I think this is related fsevents used from docquery - chokidar. When I open atom editor, it sometimes fails to load this package with a incompatible package warning on fsevents.

@jonmagic
Copy link
Contributor

jonmagic commented Sep 3, 2015

That's so weird because I'm running this locally without a problem. Let me dig into this a bit today and get back to you.

@seongjaelee
Copy link
Owner Author

I directly cloned your branch and it gave the same erorr message. Maybe it is OSX problem? But I thought Travis default test env is Linux. Are you using Windows?

@jonmagic
Copy link
Contributor

jonmagic commented Sep 4, 2015

I'm on OS X Yosemite. Didn't get a chance to look at it today, sorry :( I'll try again tomorrow.

On Sep 3, 2015, at 23:10, Seongjae Lee [email protected] wrote:

I directly cloned your branch and it gave the same erorr message. Maybe it is OSX problem? But I thought Travis default test env is Linux. Are you using Windows?


Reply to this email directly or view it on GitHub.

@jonmagic
Copy link
Contributor

jonmagic commented Sep 4, 2015

I haven't been able to get the tests to run, but in development I get the package running on the powered-by-docquery branch by:

npm install
apm rebuild
apm link /path/to/package

Run atom and hit the hot key and it comes up fine. The error I consistently get in the tests is:

Failed to require the main module of 'notational-velocity' because it requires an incompatible native module.
Run `apm rebuild` in the package directory to resolve.

@seongjaelee
Copy link
Owner Author

Could you let me know your npm -v? Is it 2.1.7? I guess your `apm -v' gives

apm  1.0.4
npm  2.13.3
node 0.10.40
python 2.7.9
git 2.3.2

@jonmagic
Copy link
Contributor

jonmagic commented Sep 8, 2015

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.

3 participants