feat: public progress API for non-lsp use cases

[willothy]

This is something I've been wanting for a while, since the only way to do it (until now) was to create a mock LSP server/client using vim.lsp.start(). This PR adds a function progress.create() which returns a reactive progress handle, allowing for progress messages to be created and easily updated by users in their configs or in other plugins.

The handle is identical to the ProgressMessage type, except it has methods for reporting / completing the progress and is reactive to changes so the user doesn't need to call notify repeatedly.

Could also be simplified if the reactivity isn't needed, so all updates would be done with ProgressHandle:report() .

Still needs docs and stuff, but figured I'd open up a PR with what I have to see if there's interest.

[j-hui]

Hey @willothy this is an awesome feature, thanks for making the PR!

A couple of organizational nits:

• Can you push this code into a separate module, under lua/fidget/progress/handle.lua? Then add that file to the workflow file to make sure lemmy-help will generate vimhelp for it.
• For better or for worse, I always add a space after the --- doc comments, so that lines begin at a column number that is a multiple of 2. I recognize this is somewhat non-standard, and I'm debating whether to fix the whole codebase's style, but for now, can you also follow this style to keep consistent?

I'll review the actual code more soon (I've been working a lot of documentation today so that's just been top of mind).

[willothy]

Sounds good! I'll make those changes now.

[j-hui]

Awesome, thanks so much for the organizational changes!

My non-nit concerns are primarily about how the Handle methods and the Message fields are combined and delegated. I've written about them inline.

I have some ideas about how I'd like this to end up, so I'll take a pass through and take a crack at addressing some of the issues I raised. But please let me know if anything I've said doesn't make sense, or if I've missed the point of something you did.

[willothy]

Refactoring now, I'm not a fan of the way I did this either - I'm not sure the reactivity is really even needed, maybe it's best to use handle:report() for everything?

[j-hui]

I'm not sure the reactivity is really even needed, maybe it's best to use handle:report() for everything?

lol I thought the very same thing while reviewing this, but then I stopped thinking that because I think it's also a very cool use of metamethods. And I think I see an elegant (ish) way to make it work. Do you mind if I take a crack at it? And if it still ends up kludgy, we can expose an interface that only supports updates using the handle:report() method.

[willothy]

I'm not sure the reactivity is really even needed, maybe it's best to use handle:report() for everything?

lol I thought the very same thing while reviewing this, but then I stopped thinking that because I think it's also a very cool use of metamethods. And I think I see an elegant (ish) way to make it work. Do you mind if I take a crack at it? And if it still ends up kludgy, we can expose an interface that only supports updates using the handle:report() method.

Of course, go ahead! Exactly, it's cool but maybe not needed. We'll see how it goes. I'm also reworking it though fyi, so I'll be pushing to this branch.

[j-hui]

Ok, I'll let you work on it first! It's your PR (:

Some notes about what I was thinking (in case these suggestions are helpful):

• an handle object that contains four fields: cancel, report, finish methods, and a rawmessage object (which is the underlying ProgressMessage
• The cancel, report, and finish methods can access the underlying message via self.rawmessage.
• a __newindex metamethod that forbids overwriting those fields + token, and that forwards ProgressMessage field updates and calls notify (like what you've already written)
• If the user does not want to trigger a reactive update, they can set handle.rawmessage.field = etc.

The main hiccup I foresee with this approach is that handle.rawmessage = wtv can still break the abstraction. Of course that can be forbidden via a read-only table pattern but I'd rather not incur more than a single layer of indirection.

[willothy]

Thanks! I've pushed a reworked version that addresses (I think) all of your feedback, and I think your suggestions align with what I've done in the latest commit.

The raw fields can technically be accessed, but I named the field _ because it really shouldn't be modified directly since that would allow the token to be changed. handle.rawmessage = wtv can definitely still break the abstraction, but I think it's not important to think about that because in Lua users will always be able to break abstractions, there's no way around it. Even if you use read-only tables someone can just use get/setmetatable, or rawget/set.

[willothy]

• If the user does not want to trigger a reactive update, they can set handle.rawmessage.field = etc.

I don't think this should be needed, if bulk updates are needed they can be done at once with :report()

[willothy]

Alright, should be good to look over again :)

[j-hui]

This looks great! I left a bunch of smaller comments/suggestions inline.

nit: I prefer to use the following format for doc comments:

---@class SomeClass no space for things that begin with @
---@field eg any
--- But space in front of regular paragraphs.
--- Like this.
---
--->lua
--- -- another exception made for vimhelp code markers
--- -- to avoid confusing vimhelp
---<
---
--- etc.

[j-hui]

Related: #70

[willothy]

This looks great! I left a bunch of smaller comments/suggestions inline.

nit: I prefer to use the following format for doc comments:

---@class SomeClass no space for things that begin with @
---@field eg any
--- But space in front of regular paragraphs.
--- Like this.
---
--->lua
--- -- another exception made for vimhelp code markers
--- -- to avoid confusing vimhelp
---<
---
--- etc.

Is the main difference the >lua and < being directly after the dashes?

[j-hui]

Is the main difference the >lua and < being directly after the dashes?

Yep, that's right.

Also, I'm testing the API doc generation using lemmy-help and it's choking a little bit on some of the things. I'm going to add a few more suggestions based on my testing with that.

[j-hui]

Some suggestions to appease lemmy-help

[willothy]

Also, I'm testing the API doc generation using lemmy-help and it's choking a little bit on some of the things. I'm going to add a few more suggestions based on my testing with that.

Yeah I've had that experience as well :/ It's tough to get lemmy-help working correctly

[j-hui]

Just pushed 0665f05, which gives you a script to try generating docs yourself (:

[willothy]

Just pushed 0665f05, which gives you a script to try generating docs yourself (:

Cool, I'll rebase and then mess around with it until it works.

[willothy]

Seems to be working now! Docs are generated.

[j-hui]

All looks good to me, thanks for the fantastic PR!

[willothy]

just a sec, there are warnings from the type annotations.

[willothy]

All looks good to me, thanks for the fantastic PR!

Thanks for building this plugin! Glad I could contribute something cool :)

[willothy]

Ok, all good to go! The syntax that worked was table<string, any> not { [string]: any }, even though lua-ls seems to work with both.

[willothy]

Awesome! Thanks for taking the time to review this, I'm excited to see what people come up with!

[j-hui]

Yeah me too! Thanks again for the idea and implementation!

[willothy]

Yeah me too! Thanks again for the idea and implementation!

Totally, my pleasure! Do you have a setup for tests, or would you accept a followup PR with CI and some tests?

[j-hui]

Do you have a setup for tests, or would you accept a followup PR with CI and some tests?

Oh I'd love that 😍 It's been on my TODO list but I haven't gotten around to it yet. (The possibility of testing was one of the reasons behind me splitting up the plugin into separate progress and notifications subsystems, and also maintaining a notifications view/abstract state separate from the window).

[willothy]

Oh I'd love that 😍 It's been on my TODO list but I haven't gotten around to it yet. (The possibility of testing was one of the reasons behind me splitting up the plugin into separate progress and notifications subsystems, and also maintaining a notifications view/abstract state separate from the window).

Cool, sounds good! Do you have a preference for plenary vs. mini for test framework?