seongjaelee / nvatom

nvatom (unpublished from atom.io)
MIT License
75 stars 14 forks source link

First attempt with DocQuery #24

Closed seongjaelee closed 9 years ago

seongjaelee commented 9 years ago

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

jonmagic commented 9 years ago

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 commented 9 years ago

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

seongjaelee commented 9 years ago

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. https://github.com/seongjaelee/notational-velocity/pull/23

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

deltaidea commented 9 years ago

@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 commented 9 years ago

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 (https://github.com/olivernn/lunr.js/issues/60#issuecomment-32913822 has more details on how this might work).

deltaidea commented 9 years ago

There're two different issues there:

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

jonmagic commented 9 years ago

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

Ahh yes, very good point :+1:

deltaidea commented 9 years ago

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!

:+1: 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 commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

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 notifications@github.com 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 commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago