Open liuzhishan opened 3 weeks ago
Thanks for the PR @liuzhishan. I think there will be some work required before we can merge. A few observations:
evil-find-file-at-point-with-line
should be backtick-quoted in docstringsif
without an else clause looks better as an and
or when
thing-at-point
code in the interactive
block.foo
than (foo nil)
let*
is not necessary when you only have one binding let
will doI'll take a look at the code in more detail once these things are ironed out. I hope that helps.
@tomdl89 thanks for the detailed reply. I'll fix the code and update the pr.
Thanks for the PR @liuzhishan. I think there will be some work required before we can merge. A few observations:
- Functions such as
evil-find-file-at-point-with-line
should be backtick-quoted in docstrings- It looks like you're using the dash library. Evil doesn't require dash, so you'll need to find a way without it
- Your indentation looks wrong on a few lines. e.g. 3738 and 3747
if
without an else clause looks better as anand
orwhen
- My inclination would be to let this function take an argument of the filename, and put the
thing-at-point
code in theinteractive
block.- When let-binding something to nil, it's usually more idiomatic to just write
foo
than(foo nil)
let*
is not necessary when you only have one bindinglet
will doI'll take a look at the code in more detail once these things are ironed out. I hope that helps.
Hi, I have update the code, please have a look if there are any other problem.
I've added a load more comments. If this is an enjoyable educational process, then by all means keep going. Equally though, if this is turning out to be more work than you had hoped for, feel free to drop this PR, make an issue, and I'll take a look when I get the time. Cheers
Hi, I think this is a enjoyable learning process. I use doom emacs, and got used to use some doom function. I'll rm the doom related code, and fix other problems.
I've added a load more comments. If this is an enjoyable educational process, then by all means keep going. Equally though, if this is turning out to be more work than you had hoped for, feel free to drop this PR, make an issue, and I'll take a look when I get the time. Cheers
Hi, thanks for the detailed comments. I have fix the code. The main change are as follows:
evil-goto-file
.string-join
, take
, without other packages dependent.find-file-at-point
.Hey @liuzhishan thanks for getting it to the point where I can test it. It seems to work as you want, but the same functionality doesn't work in vim for me. I don't have a C codebase to hand, so vim may behave differently there, but with a structure of text files as you laid out in your description, vim says E447: Cannot find file "a/a.txt" in path
and when I do :set path?
I see path=.,,,src/**,public/**
(this is what I was referring to as "path" - docs are at :h path
). I'd like to be sure we are implementing actual vim functionality before continuing with code-related feedback.
Hey @liuzhishan thanks for getting it to the point where I can test it. It seems to work as you want, but the same functionality doesn't work in vim for me. I don't have a C codebase to hand, so vim may behave differently there, but with a structure of text files as you laid out in your description, vim says
E447: Cannot find file "a/a.txt" in path
and when I do:set path?
I seepath=.,,,src/**,public/**
(this is what I was referring to as "path" - docs are at:h path
). I'd like to be sure we are implementing actual vim functionality before continuing with code-related feedback.
Hi, @tomdl89 I known where the problem is. It's not a vim functionality of gf
.
When I do :set path
in vim, I see path=.,/usr/include,,
, and gf
does not work for relative path. If I set path using :set path=.,/usr/include,,/path/to/project
, it worked for relative path under project (where /path/to/project
is the actual absolute path to project root). It seems that vim is missing the same functionality gf
for relative path.
So it's a a new functionality for emacs evil
. Should evil
be exactly the same as vim
behavior ?
Should evil be exactly the same as vim behavior ?
Usually, but not always. Evil can improve upon vim in some cases. This is especially true when dealing with something on the interface between emacs and vim.
Because there is no direct equivalent of path
in emacs, adding support for relative path for gf
is non-trivial. Your current approach hard-codes one of many possible relative filepath searching approaches, so at a minimum I'd want to document this limitation. Even better would be to introduce an evil-path
buffer-local variable which could be customized by users, but I don't think this needs adding on the first iteration of the functionality.
I'm happy to proceed with this PR, but I think the right thing to do is to implement the least surprising hard-coded approach, and then we can look at adding customizability later. For me, if I'm visiting b.h
in your example, the least surprising approach for relative paths would be to search relative to b/
. So if I wanted to navigate to a.h
with gf
, the text under the cursor would need to be ../a/a.h
. I'm very open to counter-arguments, as this isn't something I've given much thought.
Should evil be exactly the same as vim behavior ?
Usually, but not always. Evil can improve upon vim in some cases. This is especially true when dealing with something on the interface between emacs and vim.
Because there is no direct equivalent of
path
in emacs, adding support for relative path forgf
is non-trivial. Your current approach hard-codes one of many possible relative filepath searching approaches, so at a minimum I'd want to document this limitation. Even better would be to introduce anevil-path
buffer-local variable which could be customized by users, but I don't think this needs adding on the first iteration of the functionality.I'm happy to proceed with this PR, but I think the right thing to do is to implement the least surprising hard-coded approach, and then we can look at adding customizability later. For me, if I'm visiting
b.h
in your example, the least surprising approach for relative paths would be to search relative tob/
. So if I wanted to navigate toa.h
withgf
, the text under the cursor would need to be../a/a.h
. I'm very open to counter-arguments, as this isn't something I've given much thought.
Ok, let me add some comment to document the limitation. As for the least surprising approach, I think changing the traverse order would be a solution, from parent of b.h
to project root.
Hi, @tomdl89 already add comments of limitation and update the search order, please check the code.
In evil mode,
gf
doest work when path under cursor has common ancstor with current buffer file. This seems to be a common situation. For example, suppose I have the following files:and in b.h, the file
a/a.h
is included:The
gf
command was mapped tofind-file-at-point
. If I want to opena/a.h
file usinggf
inb.h
file, it doest not work.To solve this problem, I add a new function
evil-open-file-under-cursor
:find-file
.evil-find-file-at-point-with-line
as the origin logic.And also I changed the
gf
key mapping inevil-maps.el
: