feat(core): enhance org-insert-link, add completion for `~/` prefix. #749

PannenetsF · 2024-06-09T16:31:36Z

Hi guys, I am trying to hack for ~/ prefixs.

Even if the cmp is configured with org-mode and path sources, the <space>oli cannot auto-complete the ~/ prefix.
Also, this issue is the same at emacs org-mode.

It is useful as I have multiple machines to sync with. I set my org folder as ~/org/, but the ~ can be

private macOS: /User/pf/
Working Linux: /home/pf/
...

Any suggestion is appreciated. :)

seflue · 2024-06-13T18:03:00Z

Yeah, the autocompletion for ~ is somewhere down my list, I find the current behavior also quite annoying.
Actually ideal would be some relative path insertion including completion.

We try to collect all link-related string matchings in Url. If you want to continue on this, I would suggest to extend Url with a predicate is_home_relative_path and use this instead of matching manually in the business logic of find_by_filepath.

PannenetsF · 2024-06-14T15:56:07Z

Thanks for your reply and guidance. And I will fix the failed test later.

I did it in this way:
if triggered by ~/ or ./ or ../ (relative path), replace them with the real path, then the completion logic is shared.
before adding the found files to the results, replace then path back to the relative path.

So the completion logic is simplified.

PannenetsF · 2024-06-15T03:06:39Z

@seflue Hi Sebastian, I have done the refactoration. Could you take a review if convenient?

seflue · 2024-06-15T09:36:34Z

I manually tested it and this was the result:

| Test Case | works on master | works on PR |
|-----------+-----------------+-------------|
| /         | +               | +           |
| ~/        | -               | +           |
| ./        | +               | -           |
| ../       | -               | -           |

So while your targeted test case for the home path ~/ works, we have a regression with the current directory ./ and relative filepaths ../ are still not supported.

Problem: All tests are still green, so we need to improve our test coverage to catch (and fix) the regression.

If you plan on adding the relative path in a later PR, thats ok for me. Even if you fix the regression by manually testing it that would be fine for this PR (although I would very appreciate, if we can use this opportunity to cover these cases with tests). But at least the regression needs to be fixed.

PannenetsF · 2024-06-15T10:15:51Z

Thanks for the feedback.

As for the results, I am not sure how you tested it. I tested it in 2 ways : (a) <leader>oli to insert a link (b) [[ to toggle the omni completion. The results were like

Test Case	works on master	works on PR
/	+	+
~/	-	+
./	+	+
../	-	-

The behavior is caused by fs.get_real_path always returning a path without /, and I added a new optional args to it to fix it. Now I will add some tests to cover it.

seflue · 2024-06-15T10:38:04Z

As for the results, I am not sure how you tested it.
The behavior is caused by fs.get_real_path always returning a path without /, and I added a new optional args to it to fix it. Now I will add some tests to cover it.

I actually did only test <leader>oli and got the regression.

seflue · 2024-06-15T10:57:04Z

The behavior is caused by fs.get_real_path always returning a path without /, and I added a new optional args to it to fix it. Now I will add some tests to cover it.

I tested again and now it works for me. Great work! Please consider my code comment.

PannenetsF · 2024-06-15T11:19:45Z

That sounds good to me! BTW, what does the code comment mean here? I did not see code comments in the review.

I found it hard for me to write the tests (lol, as a newbie to the code structure) The completion triggers cmp instead of omni, so there is little code for reference. Do you have any suggestions about how to build the unit test?

seflue · 2024-06-15T11:45:09Z

That sounds good to me! BTW, what does the code comment mean here? I did not see code comments in the review.

You can't see my review comment I directly add to your changes? It should actually be visible within this conversation, but you can also switch to the "Files changed" tab and scroll down until you see a comment. I annotated your changes to M.get_real_path in utils.fs.lua.

I found it hard for me to write the tests (lol, as a newbie to the code structure) The completion triggers cmp instead of omni, so there is little code for reference. Do you have any suggestions about how to build the unit test?

Don't worry, it's the same for me. It's not easy to get a hang of how to implement such tests and there might be a reason why these code parts are currently not covered.

Because your changes already bring a lot of value, I would prefer for the time being to just rename the flag, merge this PR and create further PRs for improvements and test cases.

seflue · 2024-06-15T11:50:11Z

Regarding the tests, did you already found tests/plenary/org/autocompletion_spec.lua? If not, this is should be a good entry point.

PannenetsF · 2024-06-15T11:52:41Z

You can't see my review comment I directly add to your changes? It should actually be visible within this conversation, but you can also switch to the "Files changed" tab and scroll down until you see a comment. I annotated your changes to M.get_real_path in utils.fs.lua.

Seems still no comments are viewable. Not sure what happened : /

Don't worry, it's the same for me. It's not easy to get a hang of how to implement such tests and there might be a reason why these code parts are currently not covered.

Because your changes already bring a lot of value, I would prefer for the time being to just rename the flag, merge this PR and create further PRs for improvements and test cases.

Maybe I can contribute to the further test coverage in a few weeks. :)

PannenetsF · 2024-06-15T11:54:20Z

Regarding the tests, did you already found tests/plenary/org/autocompletion_spec.lua? If not, this is should be a good entry point.

Yes, I did some tests on this file and it's omni-pure. I would refer to it when building the nvim-cmp completion test.

seflue · 2024-06-15T11:55:56Z

How the omni completion in hyperlinks are tested, you can see here:

orgmode/tests/plenary/org/autocompletion_spec.lua

Lines 86 to 136 in cc51c19

    
             it('within hyperlinks', function() 
        
               setup_file('  [[') 
        
               local result = org.completion:omnifunc(1) 
        
               assert.are.same(4, result) 
        
               setup_file('  [[*some') 
        
               result = org.completion:omnifunc(1) 
        
               assert.are.same(4, result) 
        
               setup_file('  [[#val') 
        
               result = org.completion:omnifunc(1) 
        
               assert.are.same(4, result) 
        
               setup_file('  [[test') 
        
               result = org.completion:omnifunc(1) 
        
               assert.are.same(4, result) 
        
               setup_file('  [[file:') 
        
               result = org.completion:omnifunc(1) 
        
               assert.are.same(4, result) 
        
             end) 
        
             it('within file hyperlink anchors (file: prefix)', function() 
        
               setup_file('  [[file:./some/path/file.org::*') 
        
               local result = org.completion:omnifunc(1) 
        
               assert.are.same(4, result) 
        
               setup_file('  [[file:./some/path/file.org::#') 
        
               result = org.completion:omnifunc(1) 
        
               assert.are.same(4, result) 
        
               setup_file('  [[file:./some/path/file.org::') 
        
               result = org.completion:omnifunc(1) 
        
               assert.are.same(4, result) 
        
             end) 
        
             it('within file hyperlink anchors (./ prefix, headline)', function() 
        
               setup_file('  [[./1-34_some/path/file.org::*') 
        
               local result = org.completion:omnifunc(1) 
        
               assert.are.same(4, result) 
        
             end) 
        
             --TODO These tests expose a bug. Actually the expected start should be 31 as in the tests before 
        
             it('within file hyperlink anchors (./ prefix, custom_id)', function() 
        
               setup_file('  [[./1-34_some/path/file.org::#') 
        
               local result = org.completion:omnifunc(1) 
        
               assert.are.same(4, result) 
        
             end) 
        
             it('within file hyperlink anchors (./ prefix, dedicated anchor)', function() 
        
               setup_file('  [[./1-34_some/path/file.org::') 
        
               local result = org.completion:omnifunc(1) 
        
               assert.are.same(4, result) 
        
             end) 
        
           end)

Adding some test cases, which cover [[~, [[./ and [[../ should be a good starting point to cover these parts of the code base.

seflue · 2024-06-15T11:59:08Z

You can't see my review comment I directly add to your changes? It should actually be visible within this conversation, but you can also switch to the "Files changed" tab and scroll down until you see a comment. I annotated your changes to M.get_real_path in utils.fs.lua.

Seems still no comments are viewable. Not sure what happened : /

My fault, I forgot to submit the review. Is it now visible for you?

seflue · 2024-06-15T10:55:00Z

lua/orgmode/utils/fs.lua

@@ -21,7 +21,7 @@ function M.substitute_path(path_str)
 end

 ---@param filepath string
-function M.get_real_path(filepath)
+function M.get_real_path(filepath, match)


Actually I'm not a big fan of flags like these.

Questions, which come up here: Why make keeping the trailing slash optional? If the path has a trailing slash, why should it be removed? Can we make keeping it default behavior?

And if we can't, we should at least name the flag keep_trailing_slash.

Seems no other affect on tests, so I will remove the flag.

seflue · 2024-06-15T12:15:27Z

How the omni completion in hyperlinks are tested, you can see here:

orgmode/tests/plenary/org/autocompletion_spec.lua

Lines 86 to 136 in cc51c19

it('within hyperlinks', function()

setup_file(' [[')

local result = org.completion:omnifunc(1)

assert.are.same(4, result)

setup_file(' [[*some')

result = org.completion:omnifunc(1)

assert.are.same(4, result)

setup_file(' [[#val')

result = org.completion:omnifunc(1)

assert.are.same(4, result)

setup_file(' [[test')

result = org.completion:omnifunc(1)

assert.are.same(4, result)

setup_file(' [[file:')

result = org.completion:omnifunc(1)

assert.are.same(4, result)

end)

it('within file hyperlink anchors (file: prefix)', function()

setup_file(' [[file:./some/path/file.org::*')

local result = org.completion:omnifunc(1)

assert.are.same(4, result)

setup_file(' [[file:./some/path/file.org::#')

result = org.completion:omnifunc(1)

assert.are.same(4, result)

setup_file(' [[file:./some/path/file.org::')

result = org.completion:omnifunc(1)

assert.are.same(4, result)

end)

it('within file hyperlink anchors (./ prefix, headline)', function()

setup_file(' [[./1-34_some/path/file.org::*')

local result = org.completion:omnifunc(1)

assert.are.same(4, result)

end)

--TODO These tests expose a bug. Actually the expected start should be 31 as in the tests before

it('within file hyperlink anchors (./ prefix, custom_id)', function()

setup_file(' [[./1-34_some/path/file.org::#')

local result = org.completion:omnifunc(1)

assert.are.same(4, result)

end)

it('within file hyperlink anchors (./ prefix, dedicated anchor)', function()

setup_file(' [[./1-34_some/path/file.org::')

local result = org.completion:omnifunc(1)

assert.are.same(4, result)

end)

end)

Adding some test cases, which cover [[~, [[./ and [[../ should be a good starting point to cover these parts of the code base.

As I am stepping through this testfile, I now remember, that these tests just find the correct start for completion, while the actual completion is done when passing 0 to omnifunc. This confusing API is actually inherited from the original VIM.

Here we're still very short on test cases for file paths.

PannenetsF · 2024-06-15T12:16:33Z

How the omni completion in hyperlinks are tested, you can see here:

orgmode/tests/plenary/org/autocompletion_spec.lua

Lines 86 to 136 in cc51c19

it('within hyperlinks', function()

setup_file(' [[')

local result = org.completion:omnifunc(1)

assert.are.same(4, result)

setup_file(' [[*some')

result = org.completion:omnifunc(1)

assert.are.same(4, result)

setup_file(' [[#val')

result = org.completion:omnifunc(1)

assert.are.same(4, result)

setup_file(' [[test')

result = org.completion:omnifunc(1)

assert.are.same(4, result)

setup_file(' [[file:')

result = org.completion:omnifunc(1)

assert.are.same(4, result)

end)

it('within file hyperlink anchors (file: prefix)', function()

setup_file(' [[file:./some/path/file.org::*')

local result = org.completion:omnifunc(1)

assert.are.same(4, result)

setup_file(' [[file:./some/path/file.org::#')

result = org.completion:omnifunc(1)

assert.are.same(4, result)

setup_file(' [[file:./some/path/file.org::')

result = org.completion:omnifunc(1)

assert.are.same(4, result)

end)

it('within file hyperlink anchors (./ prefix, headline)', function()

setup_file(' [[./1-34_some/path/file.org::*')

local result = org.completion:omnifunc(1)

assert.are.same(4, result)

end)

--TODO These tests expose a bug. Actually the expected start should be 31 as in the tests before

it('within file hyperlink anchors (./ prefix, custom_id)', function()

setup_file(' [[./1-34_some/path/file.org::#')

local result = org.completion:omnifunc(1)

assert.are.same(4, result)

end)

it('within file hyperlink anchors (./ prefix, dedicated anchor)', function()

setup_file(' [[./1-34_some/path/file.org::')

local result = org.completion:omnifunc(1)

assert.are.same(4, result)

end)

end)

Adding some test cases, which cover [[~, [[./ and [[../ should be a good starting point to cover these parts of the code base.

I found the omnifunc is also available, and I did not find it before because I used cmp by default. So maybe cmp test is not needed for now.

seflue · 2024-06-15T12:23:08Z

lua/orgmode/utils/fs.lua

@@ -30,11 +30,8 @@ function M.get_real_path(filepath, match)
    return false
  end
  local real = vim.loop.fs_realpath(substituted)
-  if match then
-    -- if path ends with / , real needs too


Actually I find the comment useful and would not remove it.

PannenetsF · 2024-06-15T12:59:33Z

      it('should work on relative paths', function()
        local function load_file(path)
          local orgmode = require("orgmode")
          print('loading ' .. path or '')
          vim.cmd(string.format('e %s', path))
          return orgmode.files:get(path)
        end

        local files = helpers.create_agenda_files({
          "a.org",
          "b/c.org"
        }, {
          "[[./  ",
          "[[../  "
        })
        local utils = require("orgmode.utils")
        load_file(files["a.org"])
        vim.fn.cursor({ 1, 5 })
        local result = org.completion:omnifunc(0, './')

        assert.are.same({
          { menu = '[Org]', word = './a.org' },
          { menu = '[Org]', word = './b/c.org' },
        }, result)

        load_file(files["b/c.org"])
        vim.fn.cursor({ 1, 6 })

        local result = org.completion:omnifunc(0, '../')

        assert.are.same({
          { menu = '[Org]', word = '../a.org' },
          { menu = '[Org]', word = '../b/c.org' },
        }, result)
      end)

I made up some naive test on it, and it worked. I wonder if you prefer a new pr or just continue it on this thread.

the commit: a5b1f73

… or ~/ _squash: keep Url.path, and add a realpath filed fix: make sure realpath and path's ends are matched e.g. /path/to/me ../to/me /path/to/me/ ../to/me/ refactor: remove unused flag refactor: add comments on trailing slash _fix

seflue · 2024-06-15T14:30:58Z

I made up some naive test on it, and it worked. I wonder if you prefer a new pr or just continue it on this thread.

the commit: a5b1f73

As you are just continued to work on the tests, we can include these into this PR. I mentioned a separate PR just because you wrote:

Maybe I can contribute to the further test coverage in a few weeks. :)

seflue · 2024-06-15T14:33:59Z

      it('should work on relative paths', function()
        local function load_file(path)
          local orgmode = require("orgmode")
          print('loading ' .. path or '')
          vim.cmd(string.format('e %s', path))
          return orgmode.files:get(path)
        end

        local files = helpers.create_agenda_files({
          "a.org",
          "b/c.org"
        }, {
          "[[./  ",
          "[[../  "
        })
        local utils = require("orgmode.utils")
        load_file(files["a.org"])
        vim.fn.cursor({ 1, 5 })
        local result = org.completion:omnifunc(0, './')

        assert.are.same({
          { menu = '[Org]', word = './a.org' },
          { menu = '[Org]', word = './b/c.org' },
        }, result)

        load_file(files["b/c.org"])
        vim.fn.cursor({ 1, 6 })

        local result = org.completion:omnifunc(0, '../')

        assert.are.same({
          { menu = '[Org]', word = '../a.org' },
          { menu = '[Org]', word = '../b/c.org' },
        }, result)
      end)

Would you mind to split the two test cases for ./ and ../ into separate tests?

PannenetsF · 2024-06-15T15:56:50Z

Would you mind to split the two test cases for ./ and ../ into separate tests?
done with that. Maybe it is okay to do merge now?

seflue · 2024-06-15T16:21:01Z

tests/plenary/org/autocompletion_spec.lua

+          'b/c.org',
+        }, {
+          '[[./  ',
+          '[[../  ',


I don't want to be a nitpick, but I would prefer to clean each test a bit up to be specific for the case. Makes it easier for future contributions to not be distracted by code, which is actually irrelevant for the test case.

I agree with you.

The case here is mainly about paths, so the content does not really matter. So do you suggest me to add some extra info to the files?

Actually I had a misconception, how the create_agenda_files function works. The position in the file table corresponds with the position in the content table.

So I think, this cannot be improved further with reasonable amount of effort.

Fine, so we just keep it?

I also see now, that you contributed this function - I thought it already existed.

I have actually one last improvement: Can you change the interface of the function in that way, that it takes a list of tables, each one for each agenda file. And that each entry in this table is again an associative table with the keys filename and content. While filename is just a string, content would be again a table, each entry a line. This makes your function reusable for more complicated use cases with multiline content for an agenda file. I already have several test cases in mind, where this might be very useful.

And sorry for the inconvenience, I use the review functions of Github actually the first time and just found the "Review in workspace" feature, which is actually great. Wished I found it earlier.

seflue

From my side everything is good now. Thanks a lot for this contribution!

PannenetsF · 2024-06-15T16:51:30Z

:) Thanks for your kind guidance

seflue · 2024-06-15T16:54:23Z

tests/plenary/org/autocompletion_spec.lua

+          'b/c.org',
+        }, {
+          '[[./  ',
+          '[[../  ',


I also see now, that you contributed this function - I thought it already existed.

I have actually one last improvement: Can you change the interface of the function in that way, that it takes a list of tables, each one for each agenda file. And that each entry in this table is again an associative table with the keys filename and content. While filename is just a string, content would be again a table, each entry a line. This makes your function reusable for more complicated use cases with multiline content for an agenda file. I already have several test cases in mind, where this might be very useful.

seflue · 2024-06-15T16:56:34Z

tests/plenary/org/autocompletion_spec.lua

+          'b/c.org',
+        }, {
+          '[[./  ',
+          '[[../  ',


And sorry for the inconvenience, I use the review functions of Github actually the first time and just found the "Review in workspace" feature, which is actually great. Wished I found it earlier.

seflue · 2024-06-15T19:10:19Z

tests/plenary/helpers.lua

+---@return table
+local function create_agenda_files(filenames, contents)
+  -- NOTE: content is only 1 line for 1 file
+  local temp_fname = vim.fn.tempname()
+  local temp_dir = vim.fn.fnamemodify(temp_fname, ':p:h')
+  -- clear temp dir
+  vim.fn.delete(temp_dir .. '/*', 'rf')
+  local files = {}
+  local agenda_files = {}
+  for i, filename in ipairs(filenames) do
+    local fname = temp_dir .. '/' .. filename
+    fname = vim.fn.fnamemodify(fname, ':p')
+    if fname then
+      local dir = vim.fn.fnamemodify(fname, ':p:h')
+      vim.fn.mkdir(dir, 'p')
+      vim.fn.writefile({ contents[i] }, fname)
+      files[filename] = fname
+      table.insert(agenda_files, fname)
+    end
+  end
+  local cfg = vim.tbl_extend('force', {
+    org_agenda_files = agenda_files,
+  }, {})
+  local org = orgmode.setup(cfg)
+  org:init()
+  return files
+end


Suggested change

---@return table

local function create_agenda_files(filenames, contents)

-- NOTE: content is only 1 line for 1 file

local temp_fname = vim.fn.tempname()

local temp_dir = vim.fn.fnamemodify(temp_fname, ':p:h')

-- clear temp dir

vim.fn.delete(temp_dir .. '/*', 'rf')

local files = {}

local agenda_files = {}

for i, filename in ipairs(filenames) do

local fname = temp_dir .. '/' .. filename

fname = vim.fn.fnamemodify(fname, ':p')

if fname then

local dir = vim.fn.fnamemodify(fname, ':p:h')

vim.fn.mkdir(dir, 'p')

vim.fn.writefile({ contents[i] }, fname)

files[filename] = fname

table.insert(agenda_files, fname)

end

end

local cfg = vim.tbl_extend('force', {

org_agenda_files = agenda_files,

}, {})

local org = orgmode.setup(cfg)

org:init()

return files

end

---@param agenda_file {filename: string, content: string[] }[]

---@return table

local function create_agenda_files(fixtures)

-- NOTE: content is only 1 line for 1 file

local temp_fname = vim.fn.tempname()

local temp_dir = vim.fn.fnamemodify(temp_fname, ':p:h')

-- clear temp dir

vim.fn.delete(temp_dir .. '/*', 'rf')

local files = {}

local agenda_files = {}

for fixture in pairs(fixtures) do

local fname = temp_dir .. '/' .. fixture.filename

fname = vim.fn.fnamemodify(fname, ':p')

if fname then

local dir = vim.fn.fnamemodify(fname, ':p:h')

vim.fn.mkdir(dir, 'p')

vim.fn.writefile(fixture.content, fname)

files[filename] = fname

table.insert(agenda_files, fname)

end

end

local cfg = vim.tbl_extend('force', {

org_agenda_files = agenda_files,

}, {})

local org = orgmode.setup(cfg)

org:init()

return files

end

This is, what I imagine as the modified interface of the helper function.

I agree with @seflue on this. We can also add the 2nd parameter to be config, the same way we have it for create_agenda_file.

seflue · 2024-06-15T19:12:08Z

tests/plenary/org/autocompletion_spec.lua

+        local files = helpers.create_agenda_files({
+          'a.org',
+          'b/c.org',
+        }, {
+          '[[./  ',
+          '[[../  ',
+        })


Suggested change

local files = helpers.create_agenda_files({

'a.org',

'b/c.org',

}, {

'[[./ ',

'[[../ ',

})

local files = helpers.create_agenda_files({

{

filename = 'a.org',

content = { {'[[./'}},

},

{

filename = 'b.org',

content = {}

}

})

More specific test case B.

seflue · 2024-06-15T19:12:49Z

tests/plenary/org/autocompletion_spec.lua

+        local files = helpers.create_agenda_files({
+          'a.org',
+          'b/c.org',
+        }, {
+          '[[./  ',
+          '[[../  ',
+        })


Suggested change

local files = helpers.create_agenda_files({

'a.org',

'b/c.org',

}, {

'[[./ ',

'[[../ ',

})

local files = helpers.create_agenda_files({

{

filename = 'a.org',

content = {},

},

{

filename = 'b.org',

content = { {'[[../'} }

}

More specific test case A.

seflue · 2024-06-16T09:38:59Z

@kristijanhusak I think, we can merge this PR, I can also make the small cosmetic changes myself. Just a question: Are you the only person, who can approve PRs? My approval does not remove the merge block.

PannenetsF · 2024-06-16T13:32:42Z

Thanks for the edition, I didn't check mails today. lol

…

---Original--- From: "Sebastian ***@***.***> Date: Sun, Jun 16, 2024 17:20 PM To: ***@***.***>; Cc: "Yunqian ***@***.******@***.***>; Subject: Re: [nvim-orgmode/orgmode] feat(core): enhance org-insert-link, addcompletion for `~/` prefix. (PR #749) @seflue approved this pull request. In tests/plenary/helpers.lua: > ***@***.*** table +local function create_agenda_files(filenames, contents) + -- NOTE: content is only 1 line for 1 file + local temp_fname = vim.fn.tempname() + local temp_dir = vim.fn.fnamemodify(temp_fname, ':p:h') + -- clear temp dir + vim.fn.delete(temp_dir .. '/*', 'rf') + local files = {} + local agenda_files = {} + for i, filename in ipairs(filenames) do + local fname = temp_dir .. '/' .. filename + fname = vim.fn.fnamemodify(fname, ':p') + if fname then + local dir = vim.fn.fnamemodify(fname, ':p:h') + vim.fn.mkdir(dir, 'p') + vim.fn.writefile({ contents[i] }, fname) + files[filename] = fname + table.insert(agenda_files, fname) + end + end + local cfg = vim.tbl_extend('force', { + org_agenda_files = agenda_files, + }, {}) + local org = orgmode.setup(cfg) + org:init() + return files +end ⬇️ Suggested change ***@***.*** table -local function create_agenda_files(filenames, contents) - -- NOTE: content is only 1 line for 1 file - local temp_fname = vim.fn.tempname() - local temp_dir = vim.fn.fnamemodify(temp_fname, ':p:h') - -- clear temp dir - vim.fn.delete(temp_dir .. '/*', 'rf') - local files = {} - local agenda_files = {} - for i, filename in ipairs(filenames) do - local fname = temp_dir .. '/' .. filename - fname = vim.fn.fnamemodify(fname, ':p') - if fname then - local dir = vim.fn.fnamemodify(fname, ':p:h') - vim.fn.mkdir(dir, 'p') - vim.fn.writefile({ contents[i] }, fname) - files[filename] = fname - table.insert(agenda_files, fname) - end - end - local cfg = vim.tbl_extend('force', { - org_agenda_files = agenda_files, - }, {}) - local org = orgmode.setup(cfg) - org:init() - return files -end ***@***.*** agenda_file {filename: string, content: string[] }[] ***@***.*** table +local function create_agenda_files(fixtures) + -- NOTE: content is only 1 line for 1 file + local temp_fname = vim.fn.tempname() + local temp_dir = vim.fn.fnamemodify(temp_fname, ':p:h') + -- clear temp dir + vim.fn.delete(temp_dir .. '/*', 'rf') + local files = {} + local agenda_files = {} + for fixture in pairs(fixtures) do + local fname = temp_dir .. '/' .. fixture.filename + fname = vim.fn.fnamemodify(fname, ':p') + if fname then + local dir = vim.fn.fnamemodify(fname, ':p:h') + vim.fn.mkdir(dir, 'p') + vim.fn.writefile(fixture.content, fname) + files[filename] = fname + table.insert(agenda_files, fname) + end + end + local cfg = vim.tbl_extend('force', { + org_agenda_files = agenda_files, + }, {}) + local org = orgmode.setup(cfg) + org:init() + return files +end This is, what I imagine as the modified interface of the helper function. In tests/plenary/org/autocompletion_spec.lua: > + local files = helpers.create_agenda_files({ + 'a.org', + 'b/c.org', + }, { + '[[./ ', + '[[../ ', + }) ⬇️ Suggested change - local files = helpers.create_agenda_files({ - 'a.org', - 'b/c.org', - }, { - '[[./ ', - '[[../ ', - }) + local files = helpers.create_agenda_files({ + { + filename = 'a.org', + content = { {'[[./'}}, + }, + { + filename = 'b.org', + content = {} + } + }) More specific test case B. In tests/plenary/org/autocompletion_spec.lua: > + local files = helpers.create_agenda_files({ + 'a.org', + 'b/c.org', + }, { + '[[./ ', + '[[../ ', + }) ⬇️ Suggested change - local files = helpers.create_agenda_files({ - 'a.org', - 'b/c.org', - }, { - '[[./ ', - '[[../ ', - }) + local files = helpers.create_agenda_files({ + { + filename = 'a.org', + content = {}, + }, + { + filename = 'b.org', + content = { {'[[../'} } + } More specific test case A. In tests/plenary/org/autocompletion_spec.lua: > @@ -339,6 +339,39 @@ describe('Autocompletion', function() { menu = '[Org]', word = 'Title without anchor' }, }, result) end) + + it('should work on relative paths', function() + local files = helpers.create_agenda_files({ + 'a.org', + 'b/c.org', + }, { + '[[./ ', + '[[../ ', I also see now, that you contributed this function - I thought it already existed. I have actually one last improvement: Can you change the interface of the function in that way, that it takes a list of tables, each one for each agenda file. And that each entry in this table is again an associative table with the keys filename and content. While filename is just a string, content would be again a table, each entry a line. This makes your function reusable for more complicated use cases with multiline content for an agenda file. I already have several test cases in mind, where this might be very useful. In tests/plenary/org/autocompletion_spec.lua: > @@ -339,6 +339,39 @@ describe('Autocompletion', function() { menu = '[Org]', word = 'Title without anchor' }, }, result) end) + + it('should work on relative paths', function() + local files = helpers.create_agenda_files({ + 'a.org', + 'b/c.org', + }, { + '[[./ ', + '[[../ ', And sorry for the inconvenience, I use the review functions of Github actually the first time and just found the "Review in workspace" feature, which is actually great. Wished I found it earlier. — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you authored the thread.Message ID: ***@***.***>

seflue · 2024-07-02T16:29:02Z

@kristijanhusak Can we merge this? I have tested it and use it daily (is merged on my personal development branch). Would be great to bring it to upstream. I have also a further extension in mind (emacs feature), which is depending on this.

kristijanhusak

Didn't test it yet, but I'd like few things to be addressed first.

kristijanhusak · 2024-07-02T19:23:21Z

lua/orgmode/org/hyperlinks/url.lua

@@ -116,7 +117,7 @@ function Url:get_file()
  if not self:is_file() then
    return nil
  end
-  return self.path
+  return self.realpath or self.path


Add realpath to annotations.

kristijanhusak · 2024-07-02T19:27:47Z

tests/plenary/helpers.lua

+---@return table
+local function create_agenda_files(filenames, contents)
+  -- NOTE: content is only 1 line for 1 file
+  local temp_fname = vim.fn.tempname()
+  local temp_dir = vim.fn.fnamemodify(temp_fname, ':p:h')
+  -- clear temp dir
+  vim.fn.delete(temp_dir .. '/*', 'rf')
+  local files = {}
+  local agenda_files = {}
+  for i, filename in ipairs(filenames) do
+    local fname = temp_dir .. '/' .. filename
+    fname = vim.fn.fnamemodify(fname, ':p')
+    if fname then
+      local dir = vim.fn.fnamemodify(fname, ':p:h')
+      vim.fn.mkdir(dir, 'p')
+      vim.fn.writefile({ contents[i] }, fname)
+      files[filename] = fname
+      table.insert(agenda_files, fname)
+    end
+  end
+  local cfg = vim.tbl_extend('force', {
+    org_agenda_files = agenda_files,
+  }, {})
+  local org = orgmode.setup(cfg)
+  org:init()
+  return files
+end


I agree with @seflue on this. We can also add the 2nd parameter to be config, the same way we have it for create_agenda_file.

seflue · 2024-07-03T23:07:35Z

@kristijanhusak I integrated the suggested changes on my local version of this branch. Should I just create a new PR to get these changes upstream?

kristijanhusak · 2024-07-04T08:44:36Z

I have also a further extension in mind (emacs feature), which is depending on this.

@seflue how far you on this? We can merge this and integrate the suggested changes as part of that PR if you have something ready.

seflue · 2024-07-04T17:22:11Z

I have also a further extension in mind (emacs feature), which is depending on this.

@seflue how far you on this? We can merge this and integrate the suggested changes as part of that PR if you have something ready.

I just have the suggested changes ready, but didn't start on the new feature. I didn't start on it, knowing that the PR is still not merged.
If it is too much work, you can also merge this one and I make a new PR just with the missing annotation, the small refactoring of the helper method and the adjusted test cases. I will open it as soon as this one is merged.

kristijanhusak · 2024-07-04T19:01:24Z

Ok, merging this in. You can prepare the PR with the fixes.

* fix: add realpath annotation * refactor: test helper --------- Co-authored-by: Sebastian Flügge <seflue@users.noreply.github.com>

…vim-orgmode#749) * feat(core): enhance org-insert-link, add completion for relative path or ~/ _squash: keep Url.path, and add a realpath filed fix: make sure realpath and path's ends are matched e.g. /path/to/me ../to/me /path/to/me/ ../to/me/ refactor: remove unused flag refactor: add comments on trailing slash _fix * test: add unit test for relative dirs --------- Co-authored-by: fanyunqian <fanyunqian@sensetime.com>

* fix: add realpath annotation * refactor: test helper --------- Co-authored-by: Sebastian Flügge <seflue@users.noreply.github.com>

PannenetsF force-pushed the dev-org-insert-link-enhacement branch from 69896b7 to 539a3f1 Compare June 14, 2024 15:52

PannenetsF force-pushed the dev-org-insert-link-enhacement branch 2 times, most recently from 87800bb to 99d8313 Compare June 15, 2024 03:04

seflue suggested changes Jun 15, 2024

View reviewed changes

PannenetsF force-pushed the dev-org-insert-link-enhacement branch from fb38b94 to 091ea76 Compare June 15, 2024 13:06

test: add unit test for relative dirs

1d922af

seflue reviewed Jun 15, 2024

View reviewed changes

seflue approved these changes Jun 15, 2024

View reviewed changes

seflue approved these changes Jun 16, 2024

View reviewed changes

kristijanhusak requested changes Jul 2, 2024

View reviewed changes

kristijanhusak approved these changes Jul 4, 2024

View reviewed changes

kristijanhusak merged commit 47c3d92 into nvim-orgmode:master Jul 4, 2024
6 checks passed

seflue mentioned this pull request Jul 4, 2024

Little improvements to #749 #771

Merged

kristijanhusak pushed a commit that referenced this pull request Jul 4, 2024

Little improvements to #749 (#771)

95fb795

* fix: add realpath annotation * refactor: test helper --------- Co-authored-by: Sebastian Flügge <seflue@users.noreply.github.com>

feat(core): enhance org-insert-link, add completion for ~/ prefix. #749

feat(core): enhance org-insert-link, add completion for ~/ prefix. #749

Conversation

PannenetsF commented Jun 9, 2024

seflue commented Jun 13, 2024

PannenetsF commented Jun 14, 2024

PannenetsF commented Jun 15, 2024

seflue commented Jun 15, 2024

PannenetsF commented Jun 15, 2024 • edited Loading

seflue commented Jun 15, 2024

seflue commented Jun 15, 2024

PannenetsF commented Jun 15, 2024

seflue commented Jun 15, 2024

seflue commented Jun 15, 2024

PannenetsF commented Jun 15, 2024

PannenetsF commented Jun 15, 2024

seflue commented Jun 15, 2024

seflue commented Jun 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seflue commented Jun 15, 2024

PannenetsF commented Jun 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PannenetsF commented Jun 15, 2024 • edited Loading

seflue commented Jun 15, 2024 • edited Loading

seflue commented Jun 15, 2024

PannenetsF commented Jun 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seflue left a comment

Choose a reason for hiding this comment

PannenetsF commented Jun 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seflue commented Jun 16, 2024

PannenetsF commented Jun 16, 2024 via email

seflue commented Jul 2, 2024

kristijanhusak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seflue commented Jul 3, 2024

kristijanhusak commented Jul 4, 2024

seflue commented Jul 4, 2024 • edited Loading

kristijanhusak commented Jul 4, 2024

feat(core): enhance org-insert-link, add completion for `~/` prefix. #749

feat(core): enhance org-insert-link, add completion for `~/` prefix. #749

PannenetsF commented Jun 15, 2024 •

edited

Loading

seflue commented Jun 15, 2024 •

edited

Loading

PannenetsF commented Jun 15, 2024 •

edited

Loading

seflue commented Jun 15, 2024 •

edited

Loading

seflue commented Jul 4, 2024 •

edited

Loading