Create subdirs when creating new note by Sleepful · Pull Request #268 · nvim-telekasten/telekasten.nvim

Sleepful · 2023-07-06T20:45:48Z

Proposed change

Context inside commit messages and linked issues.

Details

Author: Sleepful <josepablov@gmail.com>
Date:   Thu Jul 6 14:21:23 2023 -0600

    Update check_dir_and_ask: async for vim.ui.select

    vim.ui.select is meant to be used as an asynchronous operation,
    in order to do this, it receives a callback as a third argument.

    This commit adjusts check_dir_and_ask to leverage the asynchronous
    usage of vim.ui.select, as a side effect to make this work, I also
    refactored the code that depends on the function in order to leverage
    the asynchronous operations.

    References:
    - https://github.com/stevearc/dressing.nvim/issues/80#issuecomment-1380734202
    - https://github.com/stevearc/dressing.nvim
    - https://github.com/neovim/neovim/pull/15771

    Fixes:
    - https://github.com/renerocksai/telekasten.nvim/issues/266

    One reason this bug can sneak into the codebase is because the
    default unmodified behavior of vim.ui.select is to use the
    :Messages section of neovim to ask for input and this UI is
    a synchronous blocking operation. However, this is not the
    intended usage of vim.ui.select as presented above.

commit b6ff784fbb46f170c8851be696a864e101606be5
Author: Sleepful <josepablov@gmail.com>
Date:   Thu Jul 6 14:20:47 2023 -0600

    Update README with correct configuration name

commit f2f517d0c80f1a89629bf68fc550e53070fc8da8
Author: Sleepful <josepablov@gmail.com>
Date:   Wed Jul 5 22:34:48 2023 -0600

    Create subdirs when creating new note

    previously it failed with error message

Replicate

One way to look at the broken functionality before this commit is to install dressing.nvim along with Telekasten and try to trigger check_dir function

Type of change

Bugfix (non-breaking change which fixes an issue)
New feature (thank you!)
Code quality improvements to existing code or addition of tests
Documentation update

It is difficult to split this into separate PRs

Additional information

This PR fixes or closes issue:
fixes [BUG] Cannot create a new note with a subdirectory if the subdirectory does not exist #266
fixes Missing folders are not created when creating a new note #217
same as above, but FR: [FR] Add option for subdir in create_note #205
This PR is related to issue:
Might be related to other issues, maybe not.

Checklist

I am running the latest version of the plugin.
The code change is tested and works locally.
There is no commented out code in this PR.
The code has been formatted using Stylua (a .stylua.toml file is provided)
The code has been checked with luacheck (a .luacheckrc file is provided)
The README.md has been updated according to this change.
The doc/telekasten.txt helpfile has been updated according to this change.

previously it failed with error message

vim.ui.select is meant to be used as an asynchronous operation, in order to do this, it receives a callback as a third argument. This commit adjusts check_dir_and_ask to leverage the asynchronous usage of vim.ui.select, as a side effect to make this work, I also refactored the code that depends on the function in order to leverage the asynchronous operations. References: - stevearc/dressing.nvim#80 (comment) - https://github.com/stevearc/dressing.nvim - neovim/neovim#15771 Fixes: - nvim-telekasten#266 One reason this bug can sneak into the codebase is because the default unmodified behavior of vim.ui.select is to use the :Messages section of neovim to ask for input and this UI is a synchronous blocking operation. However, this is not the intended usage of vim.ui.select as presented above.

lambtho12 · 2023-07-07T08:24:04Z

lua/telekasten.lua

+                            pinfo.calendar_info,
+                            function()
+                                opts.erase = true
+                                opts.erase_file = fname


Stylua complains here that fname is not defined properly. Why not keep pinfo.filepath?

hm, looks like this was an oversight on my part

lambtho12 · 2023-07-07T08:26:47Z

I have not reviewed the whole PR, but some preliminary tests on my side seems to indicate it works well. I activated the pipeline so any further commits will be checked directly for linting issues.

It is a rather large PR, I will try to review it by the end of the week-end and merge it for Monday.

Anyway, thank you already for this massive contribution!

Sleepful · 2023-07-09T00:45:44Z

No problem, sincerely loving this plugin.

I will add comments to the diff tomorrow, in order to make it easier to parse for you.

Sleepful · 2023-07-10T05:42:57Z

lua/telekasten.lua

 end

-local function check_dir_and_ask(dir, purpose)
+local function check_dir_and_ask(dir, purpose, callback)


this is the biggest source of change. Added callback param to check_dir_and_ask, now all callsites to check_dir_and_ask need to pass the "after selection" operations as a callback.

Sleepful · 2023-07-10T05:43:34Z

lua/telekasten.lua

                    vim.cmd('echomsg " "')
                    vim.cmd('echomsg "' .. dir .. ' created"')
                    ret = true
+                    callback(ret)


"resumes" the code that was passed as callback

Sleepful · 2023-07-10T05:43:43Z

lua/telekasten.lua

                    -- unreachable: plenary.Path:mkdir() will error out
                    tkutils.print_error("Could not create directory " .. dir)
                    ret = false
+                    callback(ret)


"resumes" the code that was passed as callback

Sleepful · 2023-07-10T05:45:32Z

lua/telekasten.lua

    else
        ret = true
+        if callback ~= nil then
+            callback(ret)


even though vim.ui.select was not called in this branch, code still needs to continue through callback

Sleepful · 2023-07-10T05:46:58Z

lua/telekasten.lua

 end

-local function global_dir_check()
+local function global_dir_check(callback)


this is one of the main functions affected by the refactor of check_dir_and_ask, therefore all functions that call global_dir_check are affected and need to pass in a callback

Sleepful · 2023-07-10T05:47:28Z

lua/telekasten.lua

-    if not global_dir_check() then
-        return
-    end
+    global_dir_check(function(dir_check)


so most changes look like this, code is the same as before but now it includes a function callback.

Sleepful · 2023-07-10T06:26:08Z

lua/telekasten.lua

-            ) .. "\n"
-        )
-    end
+    local file_dir = filepath:match("(.*/)") or ""


get directory from filepath

Sleepful · 2023-07-10T06:26:34Z

lua/telekasten.lua

+    check_dir_and_ask(file_dir, file_dir, function(dir_succeed)
+        if dir_succeed == false then
+            return
+        end
+
+        local ofile = io.open(filepath, "a")
+
+        for _, line in pairs(lines) do
+            ofile:write(
+                templates.subst_templated_values(
+                    line,
+                    title,
+                    calendar_info,
+                    uuid,
+                    M.Cfg.calendar_opts.calendar_monday
+                ) .. "\n"
+            )
+        end


this is the bit that asks to create non-existent directories from the note title

With the new callback parameter it becomes necessary to fill the nil arguments of a function if it is to receive the callback as the last argument to reproduce bug try creating a new weekly note.

lambtho12 · 2023-07-11T11:13:31Z

Thank you very much for this PR! Feel free to tackle other issues or pickup the big refactoring I started on the refact branch 😄

Sleepful added 3 commits July 5, 2023 22:34

Create subdirs when creating new note

f2f517d

previously it failed with error message

Update README with correct configuration name

b6ff784

lambtho12 requested changes Jul 7, 2023

View reviewed changes

Sleepful commented Jul 10, 2023

View reviewed changes

correction: wrong var name

c4b60d7

Sleepful commented Jul 10, 2023

View reviewed changes

lua/telekasten.lua

) .. "\n"

)

end

local file_dir = filepath:match("(.*/)") or ""

Copy link

Contributor Author

Sleepful Jul 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

get directory from filepath

Sleepful commented Jul 10, 2023

View reviewed changes

Fixes bug with new callback flow

96ceab0

With the new callback parameter it becomes necessary to fill the nil arguments of a function if it is to receive the callback as the last argument to reproduce bug try creating a new weekly note.

Sleepful requested a review from lambtho12 July 10, 2023 22:33

lambtho12 approved these changes Jul 11, 2023

View reviewed changes

lambtho12 merged commit 4a5e57e into nvim-telekasten:main Jul 11, 2023

Uh oh!

Conversation

Sleepful commented Jul 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed change

Replicate

Type of change

Additional information

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lambtho12 commented Jul 7, 2023

Uh oh!

Sleepful commented Jul 9, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Sleepful Jul 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lambtho12 commented Jul 11, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Sleepful commented Jul 6, 2023 •

edited

Loading

Sleepful Jul 10, 2023 •

edited

Loading