feat: add gitignore, license and funding file generation - #7
Open
paulo-granthon wants to merge 32 commits into
Open
feat: add gitignore, license and funding file generation#7paulo-granthon wants to merge 32 commits into
paulo-granthon wants to merge 32 commits into
Conversation
Adds the `file` context, covering three of the four file generation
items in the README.
:AgitateFileGitignore " pick from the available templates
:AgitateFileGitignore -t Rust
:AgitateFileLicense " pick from the available licenses
:AgitateFileLicense -l mit -a 'Paulo Granthon'
:AgitateFileFunding
Notes on the design:
Selection uses `vim.ui.select` rather than a dedicated buffer. Picking a
template is a single choice, not something to browse back and forth in,
and `vim.ui.select` picks up whatever ui-select plugin the user already
has without Agitate depending on one.
The license endpoints return the template verbatim, placeholders
included. Writing that out unchanged produces a LICENSE naming nobody,
which is worse than having none because it looks finished.
`apply_license_placeholders` fills both the `[year]`/`[fullname]` and
the `<year>`/`<name of author>` forms, since GitHub uses each depending
on the license.
The replacement values are escaped before substitution. `gsub` reads
`%` in a replacement string as a capture reference, so a copyright
holder containing one would have been mangled instead of written. A
test covers it.
All three commands ask before replacing an existing file, and
`FUNDING.yml` creates `.github/` if it is missing.
The four template endpoints share one `get` helper in `service.github`,
since they differ only in path and in the wording of the failure.
`MAINTAINERS.md` is left unchecked. GitHub has no template endpoint for
it and there is no settled convention for the contents, so it needs a
decision about format first. The README now records that rather than
leaving it looking merely unstarted.
The template endpoints had grown their own private `get`, written before the same helper landed on the HTTP branch. Merging brought both into the file. Drops the local copy and points the four template functions at `M.get`, so there is one definition of what a successful GET means.
There was a problem hiding this comment.
🟡 Changes recommended
The new file-generation code has a couple of confirmed error-handling and safety issues (module import failure semantics and potential empty .gitignore writes) that should be addressed before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds a new “File” context to agitate.nvim, enabling generation of common repository files via GitHub templates and local content generation, and documents/configures the new functionality.
Changes:
- Introduces
core.file+api.fileto generate.gitignore,LICENSE, and.github/FUNDING.yml(with selection viavim.ui.selectand overwrite confirmation). - Extends the GitHub service layer with endpoints for gitignore templates and licenses.
- Adds configuration/types and a new test suite covering placeholder substitution and funding content generation.
File summaries
| File | Description |
|---|---|
| tests/file_spec.lua | Adds tests for license placeholder substitution and FUNDING.yml content generation. |
| README.md | Documents new :AgitateFile* commands and updates the feature checklist/config example. |
| lua/agitate/types/config.lua | Adds file config typing (FileConfig, LicenseConfig). |
| lua/agitate/service/github.lua | Adds GitHub API helpers for gitignore templates and licenses. |
| lua/agitate/core/file.lua | Implements file generation logic (templates, placeholder substitution, overwrite prompting, file writes). |
| lua/agitate/config.lua | Adds default file.license.author config. |
| lua/agitate/api/init.lua | Loads the new file API during setup. |
| lua/agitate/api/file.lua | Registers :AgitateFileGitignore, :AgitateFileLicense, :AgitateFileFunding commands. |
Review details
- Files reviewed: 8/8 changed files
- Comments generated: 4
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+3
to
+6
| local ok, agitate_error = pcall(require, 'agitate.error') | ||
| if not ok then | ||
| return vim.notify(require('agitate.const.error').import, vim.log.levels.ERROR) | ||
| end |
Comment on lines
+3
to
+6
| local ok, agitate_error = pcall(require, 'agitate.error') | ||
| if not ok then | ||
| return vim.notify(require('agitate.const.error').import, vim.log.levels.ERROR) | ||
| end |
Comment on lines
+114
to
+118
| if not fetch_ok then | ||
| return agitate_error.throw(result) | ||
| end | ||
|
|
||
| write_file(cwd_path('.gitignore'), vim.split(result.source or '', '\n'), 'the ' .. template .. ' gitignore template') |
Three review findings. `api.file` and `core.file` carried the load guard shape that returns the result of `vim.notify`. Lua turns a nil module return into `true`, so `pcall(require, 'agitate.core.file')` succeeded with a boolean and the `file_or_err[method](...)` call that follows crashed while indexing it. Both now notify and raise, matching the rest of the plugin. `Gitignore` wrote `result.source or ''`, so an unexpected payload or a missing `source` produced an empty `.gitignore`. This command replaces an existing file after asking, which makes silently emptying one worse than refusing outright. It now refuses, as the LICENSE path already did for a missing body. The README checklist wrote `github` where the rest of the file capitalises `GitHub`.
There was a problem hiding this comment.
🟡 Changes recommended
The new file commands ignore parse_args’s incomplete return, so missing flag values (e.g., -t with no value) won’t be reported and can silently fall back to other behavior.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (2)
lua/agitate/core/file.lua:156
- parse_args can return
incompleteflags (declared flags provided without a value), but this command ignores that return. This means:AgitateFileLicense -lor-awith no value won’t be flagged as an error and may unexpectedly fall back to defaults/selection.
local parameters, leftover = parse_args({ '-l', '-a' }, optional_parameters)
if #leftover > 0 then
return agitate_error.throw('core.file.License -- Error: unrecognised arguments: ' .. table.concat(leftover, ' '))
end
lua/agitate/core/file.lua:221
- parse_args can return
incompleteflags (declared flags provided without a value), but this command ignores that return. As a result,:AgitateFileFunding -u(missing value) will silently fall back to the configured username instead of reporting the invocation error.
local parameters, leftover = parse_args({ '-u' }, optional_parameters)
if #leftover > 0 then
return agitate_error.throw('core.file.Funding -- Error: unrecognised arguments: ' .. table.concat(leftover, ' '))
end
- Files reviewed: 8/8 changed files
- Comments generated: 1
- Review effort level: Lite
All three file commands ignored the `incomplete` return from `parse_args`, so `:AgitateFileGitignore -t` fell through to the interactive selector as though no template had been requested, and the same applied to `-l` and `-a` on the license command and `-u` on the funding one. Silently doing something else is the behaviour `parse_args` grew a third return value to prevent. Each command now names the flag that needs a value.
There was a problem hiding this comment.
🟡 Changes recommended
The generated FUNDING.yml content does not validate or escape the username before embedding it into YAML, which can produce invalid output for user-provided values.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 8/8 changed files
- Comments generated: 1
- Review effort level: Lite
Comment on lines
+41
to
+46
| function M.funding_content(username) | ||
| return { | ||
| '# These are supported funding model platforms', | ||
| '', | ||
| 'github: [' .. username .. ']', | ||
| } |
The username was interpolated straight into a YAML flow sequence, `github: [<name>]`. A name containing `]` or `,` produced a file that looks written but does not parse, and nothing would have reported it, because writing the file succeeded. Validates against what GitHub actually issues: alphanumerics and single non leading, non trailing hyphens, up to 39 characters. That rules out every character that could break the sequence, so the check is about the account name being real rather than about escaping. Eleven cases: ordinary names, the ones that would break the YAML, and the ones GitHub itself would refuse.
There was a problem hiding this comment.
🟡 Changes recommended
There are confirmed correctness issues in core.file around GitHub username validation and optional config access that can cause invalid output or runtime errors.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 8/8 changed files
- Comments generated: 3
- Review effort level: Lite
| return false | ||
| end | ||
|
|
||
| return username:match('^%w[%w%-]*$') ~= nil and not username:find('%-%-') and username:sub(-1) ~= '-' |
| end | ||
|
|
||
| local token = options.github_access_token | ||
| local author = parameters['-a'] or options.file.license.author or options.github_username |
Comment on lines
+50
to
+51
| for _, name in ipairs({ 'oct]cat', 'oct,cat', 'oct cat', 'oct"cat', '' }) do | ||
| assert.is_false(file.is_valid_username(name), name) |
`file` and `file.license` are both optional in the config type, so `options.file.license.author` raises for any options table that omits them, and the `github_username` fallback the code was reaching for never got a chance to run. Navigated safely. The review also reported that `is_valid_username` allows underscores because `%w` matches them. It does not: Lua's `%w` is letters and digits only, and `octo_cat` was already rejected. Verified rather than assumed, so no change to the validator. Added the underscore case to the spec anyway, since the constraint was worth pinning and nothing covered it.
There was a problem hiding this comment.
🔵 Needs a closer look
The new core.file API docs use an incorrect optional_parameters type annotation (table<string> vs string[]|nil), which can mislead tooling/readers and should be corrected before merging.
Review details
Suppressed comments (3)
Previously missed (1) — in code that hasn't changed since the last review.
lua/agitate/core/file.lua:118
- The EmmyLua type for
optional_parametersis inaccurate here:parse_argsexpectsargs: string[]|nil(as received fromopts.fargs), sotable<string>can confuse language servers and readers.
This issue also appears in the following locations of the same file:
- line 165
- line 241
---@param optional_parameters? table<string> `-t` selects the template by name.
lua/agitate/core/file.lua:165
optional_parameterscomes fromopts.fargs(a list of strings). Usingtable<string>here doesn’t matchagitate.parse_args’sargs: string[]|nilcontract and can break typechecking/intellisense.
---@param optional_parameters? table<string> Parameters can be passed in order or explicitly
lua/agitate/core/file.lua:241
- The documented type for
optional_parametersshould be a string list (opts.fargs), nottable<string>;parse_argsalso documentsargsasstring[]|nil.
---@param optional_parameters? table<string> `-u` sets the username. Defaults to the configured one.
- Files reviewed: 8/8 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🟢 Approval recommended
The new functionality is self-contained, integrates cleanly with the existing API/config patterns, and includes focused tests for the key pure logic paths.
Review details
- Files reviewed: 8/8 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
apply_license_placeholders currently performs substitutions in an order that can unintentionally rewrite the inserted author string during later placeholder replacements.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 9/9 changed files
- Comments generated: 1
- Review effort level: Lite
| local safe_author = tostring(author):gsub('%%', '%%%%') | ||
| local safe_year = tostring(year):gsub('%%', '%%%%') | ||
|
|
||
| return (body:gsub('%[year%]', safe_year):gsub('%[fullname%]', safe_author):gsub('<year>', safe_year):gsub('<name of author>', safe_author)) |
The substitutions ran `[year]`, `[fullname]`, `<year>`, `<name of author>`. The author went in third from last, so the `<year>` pass that followed it operated on text the user had supplied: an author literally named `Team <year>` came out as `Team 2026`, with part of their own name replaced. Both year forms now run first and both author forms last, so no pass ever reads the inserted name. Covered by a test using an author containing a placeholder.
There was a problem hiding this comment.
🟡 Changes recommended
Overwrite confirmation can be skipped for existing-but-unreadable paths due to using filereadable() as the existence check.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 9/9 changed files
- Comments generated: 1
- Review effort level: Lite
Comment on lines
+71
to
+72
| local function write_file(path, lines, description) | ||
| local exists = vim.fn.filereadable(path) == 1 |
`write_file` used `vim.fn.filereadable`, which answers whether the path can be read, not whether anything is there. An existing file without read permission answered no, so the overwrite confirmation was skipped for precisely the paths where overwriting is most likely to be a mistake, and the command claims to always ask. `getftype` returns a non-empty string for anything present, readable or not.
There was a problem hiding this comment.
🟢 Approval recommended
The changes are well-scoped, have targeted test coverage for the critical pure functions, and only a minor convention-level issue was identified.
Review details
- Files reviewed: 9/9 changed files
- Comments generated: 1
- Review effort level: Lite
Comment on lines
+88
to
+90
| if vim.fn.writefile(lines, path) ~= 0 then | ||
| return agitate_error.throw('core.file -- Error: could not write ' .. path) | ||
| end |
Every error in this module names the command that raised it, except the write failure, which said `core.file`. With three commands writing three different files through one helper, that was the one message where knowing which command failed matters most. The helper now takes the caller's name.
There was a problem hiding this comment.
🟡 Changes recommended
The new file-generation paths have a few concrete robustness issues (response-shape/type validation and a doc/signature mismatch) that can lead to runtime errors or misleading documentation.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 9/9 changed files
- Comments generated: 4
- Review effort level: Lite
Comment on lines
+214
to
+220
| if not result.body then | ||
| return agitate_error.throw('core.file.License -- Error: GitHub returned no text for the `' .. key .. '` license.') | ||
| end | ||
|
|
||
| local body = M.apply_license_placeholders(result.body, os.date('%Y'), author) | ||
|
|
||
| write_file('core.file.License', cwd_path('LICENSE'), vim.split(body, '\n'), (result.name or key) .. ' license') |
Comment on lines
+163
to
+169
| github.list_gitignore_templates(token, function(list_ok, templates) | ||
| if not list_ok then | ||
| return agitate_error.throw(templates) | ||
| end | ||
|
|
||
| choose(nil, templates, 'Gitignore template', fetch) | ||
| end) |
Comment on lines
+233
to
+239
| local keys = {} | ||
| local labels = {} | ||
|
|
||
| for _, license in ipairs(licenses) do | ||
| keys[#keys + 1] = license.key | ||
| labels[license.key] = license.name or license.key | ||
| end |
`table<string>` is not valid LuaLS and describes the wrong shape: the value is `opts.fargs`, a `string[]`. And the notification split "Repository" across a conditional (`'Private r' or 'R'` followed by `'epository '`). Both have now been reported on several branches one file at a time, because each branch holds its own copy of the affected function. Applied across every file on every branch at once so they stop recurring.
Four review findings, all the same root cause: a 200 guarantees valid JSON, not the shape the code expects, and this module indexed and iterated without checking. The license body was tested for truthiness and then passed to `apply_license_placeholders`, which calls `gsub` on it. A body decoding to a number or `false` raised there instead of being reported. The template and license listings were assumed to be arrays. A non-list body would hand `vim.ui.select` something it cannot present, or raise in `ipairs`. Both now require a list, and each license entry is checked for the `key` the picker sends back. `write_file`'s documented parameters were also in the wrong order after it gained the command argument, so the docs described a different signature from the function.
There was a problem hiding this comment.
🟢 Approval recommended
The implementation aligns with the PR description, adds appropriate safety checks (overwrite confirmation, content/shape validation), and includes focused tests for the core pure logic and URL encoding.
Review details
- Files reviewed: 9/9 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
is_valid_username uses Lua’s %w class (which includes underscores), contradicting both GitHub’s username rules and the new tests expecting _ to be rejected.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
lua/agitate/core/file.lua:53
is_valid_usernamecurrently uses%w, which includes underscores in Lua patterns, so names likeocto_catwould be accepted even though GitHub usernames do not allow_(and the tests below expect it to be rejected). Use an explicit alnum character class instead of%w.
return username:match('^%w[%w%-]*$') ~= nil and not username:find('%-%-') and username:sub(-1) ~= '-'
- Files reviewed: 9/9 changed files
- Comments generated: 1
- Review effort level: Lite
Comment on lines
+58
to
+60
| -- Underscore is included deliberately: Lua's `%w` does not match it, so | ||
| -- this pins the behaviour the docstring describes. | ||
| for _, name in ipairs({ 'oct]cat', 'oct,cat', 'oct cat', 'oct"cat', 'octo_cat', '' }) do |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds the File context, covering three of the four file-generation items in the README.
Selection uses
vim.ui.selectPicking a template is a single choice, not something to browse back and forth in, so it does not need the dedicated buffer that the issue and PR lists will use.
vim.ui.selectalso picks up whatever ui-select plugin you already have (telescope, fzf, snacks) without Agitate depending on any of them.The license picker shows full names but sends keys, so you see "MIT License" and it requests
mit.License placeholders
The API returns templates verbatim, placeholders included:
Writing that out unchanged produces a LICENSE that legally names nobody while looking finished, which is worse than having none. Both forms GitHub uses are filled in,
[year]/[fullname]and<year>/<name of author>.A bug the tests caught
The replacement values are escaped before substitution.
gsubreads%in a replacement string as a capture reference, so a copyright holder containing one was being mangled rather than written:I wrote the test expecting it to pass, and it failed. Fixed, and the test stays.
Overwrite safety
All three ask before replacing an existing file.
FUNDING.ymlcreates.github/if it is missing.MAINTAINERS.mdleft uncheckedGitHub has no template endpoint for it, and there is no settled convention for the contents, so it needs a decision about format before it can be generated. The README now says that rather than leaving it looking merely unstarted.
Tests
Eight new, on the two pure functions: both placeholder styles, repeated occurrences, a string year, a body with no placeholders, the
%escaping case, and the funding content shape.The command bodies (network,
vim.ui.select,vim.fn.confirm,writefile) are not tested; that would need a UI-driving harness. The logic that decides what gets written is covered.Config