Skip to content

Update runner examples to spxrunner#142

Draft
joeykchen wants to merge 1 commit into
goplus:mainfrom
joeykchen:feature/command-runner
Draft

Update runner examples to spxrunner#142
joeykchen wants to merge 1 commit into
goplus:mainfrom
joeykchen:feature/command-runner

Conversation

@joeykchen
Copy link
Copy Markdown
Contributor

modfile: clarify runner command package syntax

@joeykchen joeykchen marked this pull request as draft March 30, 2026 14:30
@codecov
Copy link
Copy Markdown

codecov Bot commented Mar 30, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 55.03%. Comparing base (7443841) to head (46215c6).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #142      +/-   ##
==========================================
+ Coverage   54.28%   55.03%   +0.75%     
==========================================
  Files          13       13              
  Lines        1179     1201      +22     
==========================================
+ Hits          640      661      +21     
- Misses        520      521       +1     
  Partials       19       19              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request introduces a new runner directive to the gop.mod file, allowing users to specify a custom runner for their projects. The changes include the definition of the Runner struct, logic for parsing the directive within the modfile package, and comprehensive test cases for both valid and invalid usage. A review comment suggests enforcing a maximum of two arguments for the runner directive to ensure consistency and prevent misleading configurations.

Comment thread modfile/rule.go
Comment on lines +345 to +348
if len(args) < 1 {
errorf("usage: runner runnerCmdPkgPath [version]")
return
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

The current implementation doesn't check for the maximum number of arguments. A runner statement with more than two arguments (e.g., runner path version extra) will be partially processed, with the extra arguments being ignored. This can be misleading and is inconsistent with how other directives like import are handled.

It's better to enforce that runner has at most two arguments. You could also add a new test case for this scenario to ensure correctness.

Suggested change
if len(args) < 1 {
errorf("usage: runner runnerCmdPkgPath [version]")
return
}
if len(args) < 1 || len(args) > 2 {
errorf("usage: runner runnerCmdPkgPath [version]")
return
}

Comment thread modfile/rule.go
runnerVer := ""
if len(args) > 1 {
runnerVer, err = parseString(&args[1])
if err != nil {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

parseString only unquotes the value — it performs no semver/module-version format validation. An arbitrary string like "foobar" or "2.0.1" (missing the v prefix) is silently accepted. Peer directives such as xgo validate their version against modfile.GoVersionRE. Consider calling semver.IsValid from golang.org/x/mod/semver here, or at minimum document that no format enforcement is applied.

Comment thread modfile/rule.go
if proj.Runner != nil {
errorf("repeated runner statement")
return
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

There is no upper-bound guard (len(args) > 2). A directive like runner github.com/foo v1.0 extraToken silently drops extraToken. Other directives (e.g. xgo) reject extraneous arguments. Consider adding:

if len(args) > 2 {
    errorf("usage: runner runnerCmdPkgPath [version]")
    return
}

Comment thread modfile/rule.go
// per project is allowed.
// Example: runner github.com/goplus/spx/v2/cmd/spxrunner
// Example with version: runner github.com/goplus/spx/v2/cmd/spxrunner v2.0.1
// The optional version must be written as the second argument, not as path@version.
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The comment warns against path@version syntax, but isPkgPath only checks for a non-empty string not starting with . or _. A path containing @ (e.g. github.com/foo/bar@v2) passes validation silently and is stored as-is in Runner.Path. Either enforce the restriction in parsePkgPath, or update the doc to clarify it's guidance only.

Comment thread modfile/gop_test.go
runner github.com/goplus/spx/v2/cmd/spxrunner v2.0.1

require (
github.com/ajstarks/svgo v0.0.0-20210927141636-6d70534b1098
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

goxmodSpx2 now includes a runner directive, but TestParse2 never asserts that the parsed Runner fields are correct. A test like:

if proj.Runner == nil || proj.Runner.Path != "github.com/goplus/spx/v2/cmd/spxrunner" || proj.Runner.Version != "v2.0.1" {
    t.Fatal("unexpected Runner:", proj.Runner)
}

would ensure the happy path is actually exercised and guard against silent regressions.

@fennoai
Copy link
Copy Markdown

fennoai Bot commented Mar 30, 2026

Clean implementation that mirrors existing import/class directive patterns well. Error-path test coverage is thorough. Three actionable items: (1) validate the version string format (semver), (2) reject extra arguments beyond [version], and (3) add a positive-path assertion in TestParse2 to verify Runner.Path and Runner.Version are correctly parsed.

modfile: clarify runner command package syntax
@joeykchen joeykchen force-pushed the feature/command-runner branch from 99b18ab to 46215c6 Compare April 2, 2026 11:41
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.

1 participant