Add display of errors from sqlite3 command

[davidshepherd7]

Fixes #80.

stderr from sqlite3 is sent to a file, which is then displayed as a buffer if it contains any text after the command has finished. This is based on the shell-command function, which seems to be the only one which provides useful error handling (but shell-command is not general enough for us to use directly).

Additionally by using call-process instead of call-process-shell-command we get an error if the sqlite3 command cannot be found (my comment below was wrong, I didn't reset the connections properly when I tested it).

Finally I thought that helm-dash-sql was getting a bit too complicated and messy after these changes, so I moved the parsing out into a separate function.

[kidd]

Thanks for contributing. I'm a bit worried we'll be creating and deleting N temp files for every keypress (where N = the number of docsets). Not sure it's a problem, but it's worth looking at it...

[davidshepherd7]

Yeah that does sound a bit scary.

Anecdotally: I tried loading up all of my docsets at once (10 of them) and typing quickly. There was no noticeable slowdown.

More concretely I timed:

for i in {1..10000}; do
    temp="$(mktemp)"
    echo -n "" > "$temp"
    rm "$temp"
done

the results are:

core i7 laptop with SSD:  real  0m14.627s   user    0m1.913s  sys   0m14.035s
raspberry pi:                    real  7m7.943s     user    0m51.220s  sys 1m44.360s

which is about 1/1000 seconds and 1/25 seconds per file respectively. So with a really large number of docsets and an underpowered machine it could possibly be an issue.

However I'm not sure there's a good alternative. call-process can only write errors to a file and can only overwrite, not append. We could spawn a shell to do the append, but that has it's own cost...

[kidd]

Maybe, doing a sanity check when calling m-x helm-dash . Just once. and maybe just calling sqlite3 --version . When there are errors because sqlite is not found, I guess there's no point in getting the errors for every docset.

[davidshepherd7]

Oh, maybe I explained badly. The check that sqlite3 exists is done by call-process (it throws a proper emacs error if it doesn't exist) so we don't need to open any files to get that. The output that is written to a file is the stderr from running sqlite3.

There is at least some point in showing sqlite3's stderr though: if you've misnamed your docsets in e.g. (setq-local helm-dash-docsets '("Emacs Lisp") then that's where the errors will show up. Funnily I actually misnamed one of my docsets and only it discovered while testing this patch.

Maybe we should add a helm-dash-silence-sql-errors variable, and document that it might speed up completions?

[kidd]

What about having only one file created at the beginning of helm function, and then reusing it? And not deleting. just truncating it every time helm is used. Don't know how feasible it is. If it has a unique temporary name, next time we won't know which file we created on last execution. On the other side, we could use a fixed name "/tmp/helm-dash-errors.log" or something like this.

I'm just throwing ideas as they come, so don't take my word really seriously. well, you know, just ideas :)

[davidshepherd7]

I opened a stack overflow question asking about the best way of doing this. It seems like something that someone must have come across before, so hopefully there will be a simple answer like "use this function instead of call-process".

[kidd]

aha! it seems make-process is the way to go. is emacs 25 the last version? or it's still not released? (I'm using the git version).

[davidshepherd7]

No emacs 25 is still in development. The latest release is 24.5, but I think a lot of people will still be using slightly older versions from their distribution's repositories.

I think we should just leave this as it is for now, and once emacs 25 is out and reasonably well established we can switch to using make-process.

[kidd]

yup. At most, what we could do (just if we are motivated enough) to check for emacs-major-version and use one make-process or the old one.

Anyway, thanks for the contribution and the investigation. :D

[davidshepherd7]

I had some free time so I've added a custom variable to allow people to disable the capture of stderr (and the temp file isn't created if the variable is set).

The default value is t because I figure it's important for people to get an error message if something is not working when they first install helm-dash.

[davidshepherd7]

Hi again, sorry to pester you but what do you think of merging this now? The performance concerns are fixed because there's a variable to disable the potentially slow part. If you are still worried it could default to disabled. But I think it's safer to enable it so that people can get something working first, and optimise it later.

[kidd]

Hey, Sorry for the big silence. Super busy times... hopefully I'll find some time today/tomorrow to take a look at it, and merge all pending stuff.

[areina]

Hey @davidshepherd7 , thanks for your contribution! We merged it and we added a message in the debugging buffer to clarify usage and customization.

[davidshepherd7]

Great, glad to have helped :+1: