[Feat]: Improve DX

[AlejandroSuero]

Motivation

When I was working on the files, there were no rules for the linters nor lsp, lua_ls and (in my case) selene will complain about unused variables, parameters, shadowing, etc.

Proposal

• Linter: I will personally go for selene (a more updated and better approach than luacheck)

Configuration example

Linters

• Selene:

Expand to see configuration

# selene.toml or .selene.toml
std="neovim" # looks for a `neovim.yml` file to use as configuration

[rules]
global_usage = "warn"
deprecated = "warn" # If changed to `allow` it will rely in `lua_ls` diagnostics alone
multiple_statements = "warn"
incorrect_standard_library_use = "allow" # This is for cases like `string.format`, `package.config`, etc.
mixed_table = "allow"
unused_variable = "warn"
undefined_variable = "warn"

# neovim.yml
---
base: lua51 # to use lua5.1 what Neovim uses

globals:
  jit:
    any: true
  vim:
    any: true
  assert:
    args:
      - type: bool
      - type: string
        required: false
  after_each:
    args:
      - type: function
  before_each:
    args:
      - type: function
  describe:
    args:
      - type: string
      - type: function
  it:
    args:
      - type: string
      - type: function

---

• Luacheck: (pulled from plenay.nvim)

Expand to see configuration

-- .luacheckrc
-- Rerun tests only if their modification time changed.
cache = true

std = luajit
codes = true

self = false

-- Glorious list of warnings: https://luacheck.readthedocs.io/en/stable/warnings.html
ignore = {
  "212", -- Unused argument, In the case of callback function, _arg_name is easier to understand than _, so this option is set to off.
  "122", -- Indirectly setting a readonly global
}

globals = {
  "_",
  "_PlenaryLeafTable",
  "_PlenaryBustedOldAssert",
  "_AssociatedBufs",
}

-- Global objects defined by the C code
read_globals = {
  "vim",
}

exclude_files = {
  "lua/plenary/profile/lua_profiler.lua",
  "lua/plenary/profile/memory_profiler.lua",
  "lua/plenary/async_lib/*.lua",
}

files = {
  ["lua/plenary/busted.lua"] = {
    globals = {
      "describe",
      "it",
      "pending",
      "before_each",
      "after_each",
      "clear",
      "assert",
      "print",
    },
  },
  ["lua/plenary/async/init.lua"] = {
    globals = {
      "a",
    },
  },
  ["lua/plenary/async/tests.lua"] = {
    globals = {
      "describe",
      "it",
      "pending",
      "before_each",
      "after_each",
    },
  },
}

Formatter

We can also add a Makefile to use make lint and make format.

lint:
   @printf "\nRunning linter\n"
   @selene --display-style quiet --config ./selene.toml lua/llm
   # @luacheck lua/llm
   @printf "\Running formatter check\n"
   @stylua --color always -f ./.stylua.toml --check .

format:
   @printf "\nFixing all fixable formatting problems\n"
   @stylua --color always -f ./.stylua.toml .

Also the native support for .editorconfig in Neovim to ensure code style not only in lua files.

---

Integrating linters and formatters in worflows

• Selene and stylua:

name: lint

on:
  pull_request:
    branches:
      - main
    paths:
      - "lua/**"
      - "tests/**"

jobs:
  lint:
    runs-on: ubuntu-latest
    steps:
      - name: Checkout sources
        uses: actions/checkout@v4

      - name: Run selene
        uses: NTBBloodbath/[email protected]
        with:
          token: ${{ secrets.GITHUB_TOKEN }}
          args: --display-style quiet lua/llm

  style-lint:
    runs-on: ubuntu-latest
    steps:
      - name: Checkout sources
        uses: actions/checkout@v4

      - name: Lint with stylua
        uses: JohnnyMorganz/stylua-action@v4
        with:
          token: ${{ secrets.GITHUB_TOKEN }}
          version: latest
          args: --color always --check .

[!NOTE]

Both stylua and selene are installable from cargo.

We could use a cargo install package and then run make lint if using the Makefile approach.

• Luacheck and stylua:

name: lint

on:
  pull_request:
    branches:
      - main
    paths:
      - "lua/**"
      - "tests/**"

jobs:
  stylua:
    name: stylua
    runs-on: ubuntu-22.04
    steps:
      - uses: actions/checkout@v3
      - uses: JohnnyMorganz/stylua-action@v2
        with:
          token: ${{ secrets.GITHUB_TOKEN }}
          version: latest
          # CLI arguments
          args: --color always --check .

  luacheck:
    name: Luacheck
    runs-on: ubuntu-22.04
    steps:
      - uses: actions/checkout@v3

      - name: Prepare
        run: |
          sudo apt-get update
          sudo apt-get install -y luarocks
          sudo luarocks install luacheck

      - name: Lint
        run: make lint

---

Contributing rules, standards and code of conduct

Adding a CONTRIBUTING.md and a CODE_OF_CONDUCT.md for easy to follow instructions on how to change the repo.

In CONTRIBUTING.md we could specify and tell the linters and formatters, how to structure the code, if following conventional commits standards or not and some how to fork the repo just in case. We could also add a minimal.lua configuration to test errors for bugs.

minimal.lua config example

nvim -nu minimal.lua

vim.cmd([[set runtimepath=$VIMRUNTIME]])
vim.cmd([[set packpath=/tmp/nvim/lazy/]])

local lazypath = "/tmp/nvim/lazy/lazy.nvim"
if not vim.loop.fs_stat(lazypath) then
  vim.fn.system({
    "git",
    "clone",
    "--filter=blob:none",
    "https://github.com/folke/lazy.nvim.git",
    "--branch=stable", -- latest stable release
    lazypath,
  })
end

vim.opt.rtp:prepend(lazypath)

require("lazy").setup({
  {
    "huggingface/llm.nvim",
    config = function()
      require("llm").setup({
        -- cf Setup
      }),
    end,
    lazy = false,
    enabled = true,
  },
})

Some example of this here.

Issue and PR templating

Establish issues templates at leats for differentiating from BUG or FEATURE REQUEST, and adding bug or enhancement labels for easy to spot on the issues page.

[!NOTE]

With the new Github templating form system, we could create a form like issue for bugs, like ISSUE_TEMPLATE/bug_report.yml. Making sure the issuer check requirements like if it has used the minimal.lua configuration, and point them to it.

We can make required checks and areas so it won't submit unless checked or filled. Like an example of the configuration used or the Neovim version running, etc..

Some example of this here.

Adding a PR template to follow a changes made format on how it was being tested or not tested, which issue or features closes or fixes with Fixes #<issue number>.

Some example of this here.

Workflows actions

[!NOTE]

Some of these notes are unnecessary per se, but they add some flavour to the project 😁.

Labelers

We could some actions to improve labeling depending on the files touched in the PRs with actions/labeler in combination with amannn/action-semantic-pull-request

Another action that I find very useful is eps1lon/actions-label-merge-conflict, labels the PR with a label (conflicted for example) and sends a message to the PR telling the people on the PR that they need to solve conflicts with their PR.

Commits

For linting commits I find more useful and less annoying to use commitlint in CI than in hooks, allowing the person committing a change to not have to wait locally to commit changes. This action is as simple as:

# Using this inside an action with the `run` key
npm install --save-dev @commitlint/{cli,config-conventional}
echo "module.exports = { extends: ['@commitlint/config-conventional'] };" > commitlint.config.js
npx commitlint --from HEAD~1 --to HEAD --verbose

If we use conventional commits in the repo of course.

[!NOTE]

For formatting an linting code refer to Integrating linters and formatters section above.

[AlejandroSuero]

While trying to work on some test, I started with testing if the setup function works correctly.

Test results

• Testing with vusted

ok 1 - [llm.nvim tests] setup should set up the with default config
[LLM] config is already setnot ok 2 - [llm.nvim tests] setup should set up the with custom config
# ./tests/llm/setup_spec.lua @ 19
# Failure message: ./tests/llm/setup_spec.lua:36: Expected objects to be the same.
# Passed in:
# (string) 'bigcode/starcoder2-15b'
# Expected:
# (string) 'codellama:7b'
1..2


# Success: 1
# Failure: 1
#   ./tests/llm/setup_spec.lua:19 : [llm.nvim tests] setup should set up the with custom config

• Testing with plenary.nvim

Starting...
Scheduling: ./tests/llm/setup_spec.lua

========================================
Testing:        /Users/aome/dev/nvim_plugins/llm.nvim/tests/llm/setup_spec.lua
Success ||      [llm.nvim tests] setup should set up the with default config
Fail    ||      [llm.nvim tests] setup should set up the with custom config
            .../aome/dev/nvim_plugins/llm.nvim/tests/llm/setup_spec.lua:36: Expected objects to be the same.
            Passed in:
            (string) 'bigcode/starcoder2-15b'
            Expected:
            (string) 'codellama:7b'
            
            stack traceback:
                .../aome/dev/nvim_plugins/llm.nvim/tests/llm/setup_spec.lua:36: in function <.../aome/dev/nvim_plugins/llm.nvim/tests/llm
/setup_spec.lua:19>
            

Success:        1
Failed :        1
Errors :        0
========================================

Test setup

• minimal_init.lua for plenary

local plenary_dir = os.getenv("PLENARY_DIR") or "/tmp/plenary.nvim"
local is_not_a_directory = vim.fn.isdirectory(plenary_dir) == 0
if is_not_a_directory then
	vim.fn.system({ "git", "clone", "https://github.com/nvim-lua/plenary.nvim", plenary_dir })
end

vim.opt.rtp:append(".")
vim.opt.rtp:append(plenary_dir)

vim.cmd("runtime plugin/plenary.vim")
require("plenary.busted")

• setup_spec.lua

describe("[llm.nvim tests]", function()
  describe("setup", function()
    before_each(function()
      if require("llm").setup_done then
        require("llm").setup_done = false
        vim.lsp.stop_client(require("llm.language_server").client_id)
      end
    end)
    it("should set up the with default config", function()
      local llm = require("llm")
      local ok, err = pcall(llm.setup)
      assert.is_nil(err)
      assert.is_true(ok)
      local config = require("llm.config").get()
      assert.no_nil(config)
      assert.are.same("bigcode/starcoder2-15b", config.model)
    end)

    it("should set up the with custom config", function()
      local llm = require("llm")
      local opts = {
        model = "codellama:7b",
        url = "http://localhost:11434", -- llm-ls uses "/api/generate"
        -- cf https://github.com/ollama/ollama/blob/main/docs/api.md#parameters
        request_body = {
          -- Modelfile options for the model you use
          options = {
            temperature = 0.2,
            top_p = 0.95,
          },
        },
      }
      local config = require("llm.config")
      llm.setup(opts)
      assert.no_nil(config.get())
      assert.are.same(opts.model, config.get().model)
    end)
  end)
end)

As you can see I am setting the model to codellama:7b but getting back the default model bigcode/starcoder2-15b.

Maybe I am using the plugin incorrectly, but I copied the configs from the examples in the README.md.

[McPatate]

Thank you very very much for taking the time to write everything down. There is definitely a lot of room for improvement on making the project more contribution-friendly.

I've went ahead and merged #97 but rejected #98 as I feel it is a bit too opinionated to warrant adding CI checks.

Very happy to get some more contribution on this front, tests are also more than welcome. Let me know if there is anything I can do to help!

[AlejandroSuero]

@McPatate I understand, the CI part was from experience in other projects where people usually don't look at the PR and notice that conflicts have been found, and GitHub decided to remove the notification for that.

If you want me to I can setup stylua and selene checks in CI for PRs.

• For stylua there is this action JohnnyMorganz/stylua-action
• For selene there is this one NTBBloodbath/selene-action, but hasn't been updated since 3y ago, so I made mine following the aforementioned one AlejandroSuero/selene-linter-action

[!NOTE] Both stylua and selene are installable via cargo but I think this method is slower since cargo.

Both actions will just download latest release if none is found or the version using is behind and use that instead of having to build it.