Skip to content

Conversation

@bmrips
Copy link
Contributor

@bmrips bmrips commented Oct 26, 2025

Description

Inspired by @khaneliman's comment, I lifted all restrictions on deadnix and formatted the code base accordingly.

Also, I removed many redundant config = { ... } wrappings.

Checklist

  • Change is backwards compatible.

  • Code formatted with nix fmt or
    nix-shell -p treefmt nixfmt deadnix keep-sorted --run treefmt.

  • Code tested through nix run .#tests -- test-all or
    nix-shell --pure tests -A run.all.

  • Test cases updated/added. See example.

  • Commit messages are formatted like

    {component}: {description}
    
    {long description}
    

    See CONTRIBUTING for more information and recent commit messages for examples.

  • If this PR adds a new module

    • Added myself as module maintainer. See example.
    • Generate a news entry. See News
    • Basic tests added. See Tests
  • If this PR adds an exciting new feature or contains a breaking change.

    • Generate a news entry. See News

[formatter.deadnix]
command = "deadnix"
options = [ "--edit", "--no-lambda-arg", "--no-lambda-pattern-names" ]
options = [ "--edit" ]
Copy link
Collaborator

Choose a reason for hiding this comment

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

There's a reason this was added, though. We've done this before and it broke stuff.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok. The unit tests succeed at least.

Copy link
Collaborator

@khaneliman khaneliman left a comment

Choose a reason for hiding this comment

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

Not seeing anything immediately that looks like it would cause issues. Will take a look a bit more later though to see what we had to remedy after last time. I see a commit that changed how we structured args to prevent deadnix from removing an arg again in a99bddf and the rule change was done in 475d357 to handle certain files that were broken from default rules.

@@ -1,4 +1,4 @@
{ lib }:
{ ... }:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
{ ... }:

im = config.i18n.inputMethod;
in
{
config = lib.mkIf (im.enable && im.type == "hime") {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does deadnix actually remove top level configs by itself now? Interesting..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, I did this manually when I came across it.

Comment on lines -1 to -2
{ config, pkgs, ... }:

Copy link
Collaborator

Choose a reason for hiding this comment

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

bf893ad Robert Helgesson (2025-07-13 02:53):
tests: re-add module argument

These were removed as part of dead code removal, but they are actually
needed in the integration tests for comparing with the configuration
generated by the installation.

Hunk 1 of 1 @@ -0 +1,2 @@
+{ config, pkgs, ... }:
+

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could handle these cases with a # deadnix: skip pragma (documented here).

I just tried to check whether the removal of the lambda pattern lets the test fail, but currently, the fails in both cases.

Comment on lines -1 to -2
{ config, pkgs, ... }:

Copy link
Collaborator

Choose a reason for hiding this comment

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

^

@home-manager-ci home-manager-ci bot requested review from berbiche, fufexan and karaolidis and removed request for B1kku and MunsMan October 27, 2025 07:23
@home-manager-ci home-manager-ci bot requested a review from sumnerevans October 27, 2025 07:42
Copy link
Member

@mipmip mipmip left a comment

Choose a reason for hiding this comment

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

smug.nix 👍

Copy link
Contributor

@KarlJoad KarlJoad left a comment

Choose a reason for hiding this comment

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

mbsync/default.nix looks fine. If it passes unit tests, then 👍

@aguirre-matteo
Copy link
Member

element-desktop.nix 👍

Copy link
Contributor

@lafrenierejm lafrenierejm left a comment

Choose a reason for hiding this comment

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

👍 for modules/programs/ripgrep-all.nix

Copy link
Member

@Scrumplex Scrumplex left a comment

Choose a reason for hiding this comment

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

sway/i3 and wlogout LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.