Skip to content

refactor: subdirectory layout, mod.nu entry point, test suite#1

Open
danielbodnar wants to merge 6 commits intomainfrom
feat/restructure-and-tests
Open

refactor: subdirectory layout, mod.nu entry point, test suite#1
danielbodnar wants to merge 6 commits intomainfrom
feat/restructure-and-tests

Conversation

@danielbodnar
Copy link
Copy Markdown
Owner

Summary

  • Moves dagger.nucompletions/dagger.nu to make room for future commands, modules, etc.
  • Adds mod.nu root entry point following the nushell-modules pattern (export use completions/dagger.nu *)
  • Adds tests/completions.test.nu with 13 passing tests

Test suite covers

Category Tests
Helper values sdks, progress formats, compat, shell names, SPDX licenses, LLM models, subcommands, toolchain subcommands
Descriptions Every table-form helper must have non-empty descriptions
Extern registration All 17 top-level + 5 toolchain externs registered after use
Uniqueness No duplicate externs or completion values
Integration mod.nu correctly re-exports everything

Design note: export def for nu-complete helpers

The reference nushell-modules repo uses non-exported helpers. For this completions-focused repo, export def is the right call — the helpers are the public API (users can call nu-complete dagger sdks | get value to inspect options). This also avoids subshell gymnastics in tests.

Install path change

# Before
use /path/to/dagger.nu *

# After
use /path/to/dagger.nu/mod.nu *
# or
use /path/to/dagger.nu/completions/dagger.nu *

- completions/dagger.nu: move from root, export nu-complete helpers
- tests/completions.test.nu: 12 tests covering all completion helpers
  and registered extern commands (std/assert, no mutable closures)
- README: update install path
nu-complete helpers are exported (export def) — appropriate for a
completions module where helpers are part of the public API.
Tests use top-level `use` so scope commands reflects parser-time
scope and helpers are callable without subshell gymnastics.

Also adds mod.nu entry point following nushell-modules convention.
@danielbodnar danielbodnar marked this pull request as ready for review March 1, 2026 02:44
Copilot AI review requested due to automatic review settings March 1, 2026 02:44
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Refactors the repository into a more nushell-module-friendly layout by introducing a root mod.nu entrypoint, moving the completions into a subdirectory, and adding a Nushell test suite to validate exported helpers/externs and re-export behavior.

Changes:

  • Add mod.nu as the root module entrypoint that re-exports the completions module.
  • Export nu-complete helper defs from completions/dagger.nu and add a new tests/completions.test.nu suite.
  • Update README manual install instructions to reflect the new file layout.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File Description
tests/completions.test.nu Adds a Nushell test runner validating helper outputs, extern registration, uniqueness, and mod.nu integration.
mod.nu Introduces root entrypoint that export uses the completions module.
completions/dagger.nu Switches completion helper defs to export def to make them part of the module’s public surface.
README.md Updates manual installation guidance for the new directory layout.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread tests/completions.test.nu Outdated
Comment on lines +156 to +157
let out = ^nu --no-config-file -c $"
use '/workspaces/code/github.com/danielbodnar/dagger.nu/mod.nu' *
Copy link

Copilot AI Mar 1, 2026

Choose a reason for hiding this comment

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

test mod.nu entry point hard-codes an absolute workspace path (/workspaces/code/github.com/danielbodnar/dagger.nu/mod.nu). This will fail for anyone running the tests outside that specific directory layout (CI, local clones, Windows). Compute the module path dynamically from the test file location (e.g., via $env.FILE_PWD + path join + path expand) and interpolate that into the ^nu -c string instead of embedding a fixed path.

Suggested change
let out = ^nu --no-config-file -c $"
use '/workspaces/code/github.com/danielbodnar/dagger.nu/mod.nu' *
let mod-path = ([$env.FILE_PWD ".." "mod.nu"] | path join | path expand)
let out = ^nu --no-config-file -c $"
use '($mod-path)' *

Copilot uses AI. Check for mistakes.
Comment thread README.md
Comment on lines +38 to 42
Clone the repo, then add to `config.nu`:

```nushell
use dagger.nu *
```

Or source it directly:

```nushell
use /path/to/dagger.nu *
use /path/to/dagger.nu/completions/dagger.nu *
```
Copy link

Copilot AI Mar 1, 2026

Choose a reason for hiding this comment

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

README manual install now points users at use /path/to/dagger.nu/completions/dagger.nu *, but this PR introduces mod.nu specifically as the stable root entry point. Consider updating the README to recommend use /path/to/dagger.nu/mod.nu * (and optionally mention the completions file as an alternative) to keep documentation consistent with the new layout and intended public entrypoint.

Copilot uses AI. Check for mistakes.
@danielbodnar
Copy link
Copy Markdown
Owner Author

@copilot open a new pull request to apply changes based on the comments in this thread

Copy link
Copy Markdown

Copilot AI commented Mar 21, 2026

@danielbodnar I've opened a new pull request, #2, to work on those changes. Once the pull request is ready, I'll request review from you.

Copilot AI and others added 3 commits March 21, 2026 03:58
fix: portable mod.nu path in tests, recommend mod.nu in README
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread tests/completions.test.nu

# Top-level use — brings externs and helpers into parser scope so
# scope commands reflects them and helper calls resolve without a subshell.
use ../completions/dagger.nu *
Copy link

Copilot AI Mar 23, 2026

Choose a reason for hiding this comment

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

use ../completions/dagger.nu * is a brittle relative path. When running the test as documented (nu tests/completions.test.nu from the repo root), this resolves to ../completions/... and can fail depending on how use resolves relative paths. Build the path from $env.FILE_PWD (like the mod.nu test does) so the suite works regardless of the current working directory.

Suggested change
use ../completions/dagger.nu *
use ($env.FILE_PWD | path join ".." "completions" "dagger.nu") *

Copilot uses AI. Check for mistakes.
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.

3 participants