Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: extend lz.n with custom handlers #17

Merged
merged 1 commit into from
Jun 24, 2024
Merged

feat: extend lz.n with custom handlers #17

merged 1 commit into from
Jun 24, 2024

Conversation

BirdeeHub
Copy link
Contributor

@BirdeeHub BirdeeHub commented Jun 21, 2024

added require('lz.n').register_handler(lz.n.HandlerSpec)

added lz.n.Plugin.extras for extra spec items from custom handlers.

Added type declarations to avoid throwing warnings from adding extra items to the spec

This was necessary because require('lz.n.handler').init must call the custom handler, which cant reasonably be done from outside of the plugin.

lua/lz/n/spec.lua Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Copy link
Member

@mrcjkb mrcjkb left a comment

Choose a reason for hiding this comment

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

Thanks 🙏

Some notes (on top of my other comments):

  • Please amend the commit according to the conventional commit specification.
    Otherwise, CI won't pick it up for the changelog and semver version.
  • We should also expose the lz.n.loader.load() function as part of the public API
    (via the lz.n module). Handlers need to be able to call it, because it takes care of properly deregistering the plugins.
  • This needs to be tested. Perhaps a very simple test handler that can be invoked from a callback.
    See the other tests in the spec/ directory for inspiration.
    I use busted for testing. Since you use Nix, you can just run busted or luarocks test from within the devShell.
    You can also run everything (linting, type checking, ...) with nix flake check -Lv.

lua/lz/n/meta.lua Outdated Show resolved Hide resolved
lua/lz/n/meta.lua Outdated Show resolved Hide resolved
lua/lz/n/meta.lua Outdated Show resolved Hide resolved
lua/lz/n/meta.lua Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@mrcjkb
Copy link
Member

mrcjkb commented Jun 21, 2024

Checks are failing.
If you enter the devShell, it'll set up pre-commit hooks :)

@BirdeeHub

This comment was marked as abuse.

@BirdeeHub BirdeeHub changed the title Added the ability to extend lz.n with custom handlers. feat: extend lz.n with custom handlers. Jun 22, 2024
@BirdeeHub BirdeeHub changed the title feat: extend lz.n with custom handlers. feat: extend lz.n with custom handlers Jun 22, 2024
@BirdeeHub

This comment was marked as abuse.

README.md Outdated Show resolved Hide resolved
lua/lz/n/spec.lua Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
lua/lz/n/spec.lua Outdated Show resolved Hide resolved
lua/lz/n/spec.lua Outdated Show resolved Hide resolved
@mrcjkb
Copy link
Member

mrcjkb commented Jun 22, 2024

Im not good at writing tests due to lack of experience but thats next.

It's one of the most valuable skills to practise 😄

Please don't hesitate to ask for help if you get stuck. I suggest looking at lz_n_spec.lua for reference.
But the spec should go in its own register_handler_spec.lua file.

@BirdeeHub

This comment was marked as abuse.

lua/lz/n/meta.lua Outdated Show resolved Hide resolved
spec/register_handler_spec.lua Outdated Show resolved Hide resolved
spec/register_handler_spec.lua Outdated Show resolved Hide resolved
spec/register_handler_spec.lua Outdated Show resolved Hide resolved
spec/register_handler_spec.lua Show resolved Hide resolved
@BirdeeHub

This comment was marked as abuse.

@BirdeeHub

This comment was marked as abuse.

Copy link
Member

@mrcjkb mrcjkb left a comment

Choose a reason for hiding this comment

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

Just a few nitpicks 😃

README.md Outdated Show resolved Hide resolved
lua/lz/n/handler/init.lua Outdated Show resolved Hide resolved
lua/lz/n/handler/init.lua Outdated Show resolved Hide resolved
lua/lz/n/handler/init.lua Outdated Show resolved Hide resolved
lua/lz/n/handler/init.lua Outdated Show resolved Hide resolved
@BirdeeHub

This comment was marked as abuse.

added require('lz.n').register_handler(lz.n.HandlerSpec)

added lz.n.Plugin.extras for extra spec items from custom handlers.

feat: extend lz.n with custom handlers

feat: extend lz.n with custom handlers

added tests

Update spec/register_handler_spec.lua

Co-authored-by: Marc Jakobi <[email protected]>

Update spec/register_handler_spec.lua

Co-authored-by: Marc Jakobi <[email protected]>

feat: extend lz.n with custom handlers

Update README.md

Co-authored-by: Marc Jakobi <[email protected]>

Update lua/lz/n/handler/init.lua

Co-authored-by: Marc Jakobi <[email protected]>
Copy link
Member

@mrcjkb mrcjkb left a comment

Choose a reason for hiding this comment

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

LGTM 🎉 🚀

@mrcjkb mrcjkb merged commit d61186f into nvim-neorocks:master Jun 24, 2024
2 checks passed
@BirdeeHub

This comment was marked as abuse.

@BirdeeHub

This comment was marked as abuse.

@BirdeeHub

This comment was marked as abuse.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants