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

[PannenetsF]

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

1. private macOS: /User/pf/
2. Working Linux: /home/pf/
3. ...

Any suggestion is appreciated. :)

[seflue]

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]

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]

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

[seflue]

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]

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]

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]

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]

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]

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]

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

[PannenetsF]

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]

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]

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

https://github.com/nvim-orgmode/orgmode/blob/cc51c1914e3f0bf20bd543a7176df5bab12ec247/tests/plenary/org/autocompletion_spec.lua#L86-L136

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

[seflue]

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]

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

https://github.com/nvim-orgmode/orgmode/blob/cc51c1914e3f0bf20bd543a7176df5bab12ec247/tests/plenary/org/autocompletion_spec.lua#L86-L136

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]

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

https://github.com/nvim-orgmode/orgmode/blob/cc51c1914e3f0bf20bd543a7176df5bab12ec247/tests/plenary/org/autocompletion_spec.lua#L86-L136

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.

[PannenetsF]

      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: https://github.com/nvim-orgmode/orgmode/commit/a5b1f73698badf29def84739cba1995e6368c589

[seflue]

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]

      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]

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?

[PannenetsF]

:) Thanks for your kind guidance

[seflue]

@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]

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]

@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.

[seflue]

@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]

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]

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]

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