Conversation
fwiesel
force-pushed
the
optional-nix-and-direnv
branch
from
c481910 to
b6c7631
Compare
majewsky
left a comment
majewsky
left a comment
There was a problem hiding this comment.
Two small nitpicks below. To clarify regarding the intent, what does "break other people's workflows" mean? The .envrc is written in a way where it should not cause problems for people who do not have Nix installed. Do you mean that the problem is that go-m-m generating an .envrc file overwrites an existing custom one?
fwiesel
force-pushed
the
optional-nix-and-direnv
branch
from
2a0356c to
4e047fd
Compare
There was a problem hiding this comment.
Pull request overview
Adds configuration switches to make Nix shell generation and direnv integration optional, addressing workflow concerns while preserving the existing default behavior.
Changes:
- Extend
nixconfiguration withenabledandwriteEnvRctoggles. - Update Nix shell rendering to skip generating
shell.nixand/or.envrcbased on config. - Add tests covering the new configuration combinations and update README docs.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| internal/nix/nix-shell.go | Respect nix.enabled and nix.writeEnvRc when generating shell.nix / .envrc. |
| internal/nix/nix-shell_test.go | New test suite covering enable/disable behavior and package/variable inclusion. |
| internal/core/config.go | Add Enabled and WriteEnvRc fields to NixConfig with YAML tags. |
| README.md | Document the new nix.enabled and nix.writeEnvRc settings. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func assertFileExists(t *testing.T, path string, shouldExist bool) { | ||
| t.Helper() | ||
| _, err := os.Stat(path) | ||
| exists := !os.IsNotExist(err) |
There was a problem hiding this comment.
assertFileExists treats any os.Stat error other than IsNotExist as “file exists”. That can mask real failures (e.g., permission/IO errors) and make the test pass incorrectly. Consider failing the test on unexpected errors (or at least treating them as non-existent) instead of assuming the file exists.
| exists := !os.IsNotExist(err) | |
| var exists bool | |
| if err == nil { | |
| exists = true | |
| } else if os.IsNotExist(err) { | |
| exists = false | |
| } else { | |
| t.Fatalf("Failed to stat %s: %v", path, err) | |
| } |
Well, both overwriting it and having it checked in makes it difficult to use the |
Nix doesn't add much overhead, but writing the .envrc to activate nix-shell complicates the use of direnv for other uses. Make both configurable, but keep the existing default mostly out of consistency reasons. Update internal/nix/nix-shell_test.go Co-authored-by: Stefan Majewsky <stefan.majewsky@sap.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
fwiesel
force-pushed
the
optional-nix-and-direnv
branch
from
55c535d to
2afbdf3
Compare
Merging this branch will increase overall coverage
Coverage by fileChanged files (no unit tests)
Please note that the "Total", "Covered", and "Missed" counts above refer to code statements instead of lines of code. The value in brackets refers to the test coverage of that file in the old version of the code. Changed unit test files
|
|
I've updated the commit message. |
Nix doesn't add much overhead, but writing the .envrc to activate
nix-shell complicates the use of direnv for other uses. Make both
configurable, but keep the existing default mostly out of consistency
reasons.