Dropped f dependency

[phikal]

Hi again,

during my last pull request I was reading through the code, and was a bit disappointed to see how much the f, s and dash libraries are bing used. I personally, consider it bad elisp style, and wanted to suggest dropping them incrementally for the sake of legibility, speed, and good style.

My issue is the following: If one takes a look at how many of the f-... functions are defined, they are often just aliases for existing and perfectly good Emacs functions (for example f-exists? and file-exists-p). In other cases they promote an inefficient style that ignores some of the strengths of Elisp, such as using string over buffers (see my dumb-jump-read-config` rewrite for example).

If you're ok with my proposition to stick more to "vanilla" Elsip, I'll submit a few more push requests soon to remove the other two libraries and possibly rewrite a few functions to avoid unnecessary consing where possible. If not, it's no problem at all.

[phikal]

I'm reading the test results, and I have a few comments:

1. In dumb-jump-cpp-test1, the issue is that dumb-jump-go is returning the relative file name, as opposed to the absolute file name. I don't think this is strictly speaking a mistake, but I'll try to fix it.
2. In dumb-jump-exclude-path-test the files are returned in the reverse order which is trivial to fix, but then I don't understand why /home/travis/dumb-jump/test/data/proj1/ignored2 is expected, when the file includes /ignore2, an absolute file path?

[jacktasia]

a bit disappointed to see how much the f, s and dash libraries are bing used. I personally, consider it bad elisp style, and wanted to suggest dropping them incrementally for the sake of legibility, speed, and good style.

I've definitely considered dropping these before because I'd much rather use built-ins then bring in a whole dependency, but in practice, this never worked very well particularly for supporting older versions of emacs. While some are merely better-named aliases for builtins some are doing quite a bit to ensure backward compatibility (if I remember correctly).

If you can get all the tests to pass without adding any complicated helper functions into the code base then I am definitely supportive and will merge. I just don't want to add complexity or duplicate (then need to separately maintain) existing code that already exists in these libraries.

[jacktasia]

I'm reading the test results, and I have a few comments:

1. In dumb-jump-cpp-test1, the issue is that dumb-jump-go is returning the relative file name, as opposed to the absolute file name. I don't think this is strictly speaking a mistake, but I'll try to fix it.
2. In dumb-jump-exclude-path-test the files are returned in the reverse order which is trivial to fix, but then I don't understand why /home/travis/dumb-jump/test/data/proj1/ignored2 is expected, when the file includes /ignore2, an absolute file path?

1. I can't remember the details but for the most part if I am looking for an absolute path it's for a good reason and to get around weirdness with projects that have duplicated path parts (multiple /srcs etc)
2. The -/ignored2 is not actually an absolute file path because searches are always relative to the directory/file they are explicitly searching. That is, if you try ag's --ignore-dir option it will treat both ways the same, which if I remember correctly is why I added both versions.

[phikal]

The -/ignored2 is not actually an absolute file path because searches are always relative to the directory/file they are explicitly searching. That is, if you try ag's --ignore-dir option it will treat both ways the same, which if I remember correctly is why I added both versions.

Ok, but then dumb-jump-include-path-test shouldn't be expecting an absolute path (/etc/var/some/code). I've been jumping back and forth between fixing dumb-jump-include-path-test and dumb-jump-exclude-path-blank-test, because the behaviour of the one seems to contradict the other. The only way to fix this would be to handle includes and excludes differently, but that seems like it would only provoke more confusion.

Edit: Nevermind, I re-read the old code and noticed that there already was a different handling for included and excluded files. The only issue now is that dumb-jump-read-config-remote-test aborts. Unless I'm misunderstanding something, the test isn't actually testing a remote file though, because the return value of f-read is locally mocked, instead of TRAMP being used.

[jacktasia]

@phikal Thanks for doing this! This is looking good. I am thinking of merging this soon. Do you have any additional concerns before I do?

[phikal]

No, I have nothing more to add to this pull request. The only caveat would be to mention that I'm close to finishing a near 100% rewrite of the packet. As mention above, I started rewriting the code to drop s and dash, one thing lead to another and now I have a 5000+ line diff. I'm uncertain if submitting it as a pull request even makes sense, so if you're not too interested in that, merging this seems like a good first step.

[jacktasia]

No, I have nothing more to add to this pull request. The only caveat would be to mention that I'm close to finishing a near 100% rewrite of the packet. As mention above, I started rewriting the code to drop s and dash, one thing lead to another and now I have a 5000+ line diff. I'm uncertain if submitting it as a pull request even makes sense, so if you're not too interested in that, merging this seems like a good first step.

Wow, that's quite the change. Since you did the work you might as well open a PR. I'd be very curious to see what all changed, but to be perfectly honest I might not accept that drastic of a change.

That said, I'll keep testing this PR and probably merge this one soon. Thanks again!

[phikal]

I'll open a PR when it's stable. Can't grantee when that will be though :/

---

In case you're interesting, here's a snapshot of the current state: https://0x0.st/iQE0.el. It doesn't work yet and I have to still rework dumb-jump--result-follow and dumb-jump-process-results and adapt few interactive commands, but the design should stay the same. The main additions to notice is the usage of EIEIO and that the search-process is started asynchronously.

[jacktasia]

Thanks!