Skip to content

fix: portable mod.nu path in tests, recommend mod.nu in README#2

Merged
danielbodnar merged 3 commits intofeat/restructure-and-testsfrom
copilot/sub-pr-1
Mar 23, 2026
Merged

fix: portable mod.nu path in tests, recommend mod.nu in README#2
danielbodnar merged 3 commits intofeat/restructure-and-testsfrom
copilot/sub-pr-1

Conversation

Copy link
Copy Markdown

Copilot AI commented Mar 21, 2026

Two issues from PR review: hardcoded absolute path in the mod.nu integration test broke on any non-workspace clone, and the README still pointed users at the completions file directly instead of the new stable mod.nu entrypoint.

Changes

  • tests/completions.test.nu — compute mod.nu path dynamically from $env.FILE_PWD instead of embedding a hardcoded workspace path:

    let mod-path = ([$env.FILE_PWD ".." "mod.nu"] | path join | path expand)
    let out = ^nu --no-config-file -c $"
        use '($mod-path)' *
        ...
  • README.md — promote mod.nu to the recommended manual install entry point; list the bare completions file as an alternative:

    # Recommended
    use /path/to/dagger.nu/mod.nu *
    
    # Alternative
    use /path/to/dagger.nu/completions/dagger.nu *

✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.

Copilot AI changed the title [WIP] Refactor subdirectory layout and add mod.nu entry point fix: portable mod.nu path in tests, recommend mod.nu in README Mar 21, 2026
Copilot AI requested a review from danielbodnar March 21, 2026 03:59
@danielbodnar danielbodnar marked this pull request as ready for review March 22, 2026 08:01
Copilot AI review requested due to automatic review settings March 22, 2026 08:01
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

This PR fixes portability issues in the Nushell integration test by removing a hardcoded absolute module path, and updates the README to recommend the stable mod.nu entrypoint for manual installs.

Changes:

  • Update tests/completions.test.nu to compute the repository mod.nu path dynamically from $env.FILE_PWD.
  • Update README.md to recommend mod.nu as the primary manual-install entry point (with completions/dagger.nu as an alternative).

Reviewed changes

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

File Description
tests/completions.test.nu Removes hardcoded absolute path; builds mod.nu path dynamically for portability.
README.md Documents mod.nu as the recommended entrypoint; keeps direct completions import as an alternative.

💡 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 +158
let mod-path = ([$env.FILE_PWD ".." "mod.nu"] | path join | path expand)
let out = ^nu --no-config-file -c $"
use '/workspaces/code/github.com/danielbodnar/dagger.nu/mod.nu' *
use '($mod-path)' *
Copy link

Copilot AI Mar 22, 2026

Choose a reason for hiding this comment

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

The new variable name mod-path is inconsistent with the rest of this test file, which uses snake_case (e.g., all_tests). Consider renaming to mod_path (and updating the interpolation) for consistency and to avoid any potential parsing ambiguity around - in variable references.

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

@copilot apply changes based on the comments in this thread

Copy link
Copy Markdown
Author

Copilot AI commented Mar 22, 2026

@copilot apply changes based on the comments in this thread

Renamed mod-pathmod_path in commit 5cba432 to match the snake_case convention used throughout the test file.

@danielbodnar danielbodnar merged commit c9446f3 into feat/restructure-and-tests Mar 23, 2026
2 checks passed
@danielbodnar danielbodnar deleted the copilot/sub-pr-1 branch March 23, 2026 01:01
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