treewide: get rid of `with lib;` and friends #209325

AndersonTorres · 2023-01-06T15:34:00Z

Description of changes

Tracks

#208242

Things done

AndersonTorres · 2023-01-06T20:36:56Z

I am cogitating if it is better to split this per file.

thiagokokada · 2023-01-08T13:05:07Z

nixos/maintainers/scripts/cloudstack/cloudstack-image.nix

@@ -2,8 +2,6 @@

 { config, lib, pkgs, ... }:

-with lib;


Would be nice to have a PR with only those removals, since this one can be merged really fast.

I am not following.
My idea is that each commit should be buildable. If I merely remove the with lib and merge, obviously it will generate errors like "mkDefault not defined".

What do you mean? That this commit is broken them? Because it is clear that a few files only remove with lib; and do nothing else.

I am talking about only about the files that remove with lib; and do nothing else BTW.

Yes, same idea from me. If we can remove a with anywhere without having regressions we can immediately merge it. This can also be done treewide to make mistakes in commits unlikely.

This change is uncontroversial for me

SuperSandro2000 · 2023-01-08T23:56:11Z

nixos/modules/hardware/all-firmware.nix

+  inherit (lib)
+    literalExpression mdDoc mkIf mkMerge mkOption mkRenamedOptionModule types;


IMO this is not an improvement over just prefixing things with lib. or using with lib more locally.

At this first moment I am more interested in make the expressions compile. More cosmetic modifications can be done later.

I am talking about the general idea of your change. If the next person (for example me) is going to revert this again because maintaining a list of used functions is not worth it, then we didn't achieve anything.

The "idea" of my change is just getting rid of with.

For this draft phase, I believe it is easier to use inherit (lib) every thing that was called;. It will help a future automation.

I have no problem in prefixing lib. This can be done with the help of an Emacs quick&dirty magic keystroke. Indeed I will do this tonight.

I am more reticent about "using lib more locally". It is becoming ridiculous to see "mkDefault" in a line and "lib.mdDoc" in the next one.

And it is depressing to look at meta = with pkgs.lib.maintainers; { maintainers = []; };. Such a heavy machinery to empty lists?

And it is depressing to look at meta = with pkgs.lib.maintainers; { maintainers = []; };. Such a heavy machinery to empty lists?

It is empty to convey that the package needs a maintainer.

It is empty to convey that the package needs a maintainer.

What is the difference for meta.maintainers = [ ];?

SuperSandro2000

We can't merge this before we don't have some consensus otherwise the next person is going to revert it and then we all are not making good use of our time.

I would suggest that we split out all the uncontroversial changes into separate, treewide PRs so that they can be merged faster and the merge conflicts are reduced.

Also please do not self merge this PR.

SuperSandro2000 · 2023-01-13T12:36:08Z

nixos/maintainers/scripts/cloudstack/cloudstack-image.nix

@@ -2,8 +2,6 @@

 { config, lib, pkgs, ... }:

-with lib;


Yes, same idea from me. If we can remove a with anywhere without having regressions we can immediately merge it. This can also be done treewide to make mistakes in commits unlikely.

This change is uncontroversial for me

SuperSandro2000 · 2023-01-13T12:37:28Z

nixos/modules/hardware/gkraken.nix

-    services.udev.packages = with pkgs; [
-      gkraken
-    ];
+    services.udev.packages = [ pkgs.gkraken ];


For me the line before the change was so easy to understand that I can parse it at first glimpse without thinking. I can no longer do that with pkgs. especially if there are multiple.

SuperSandro2000 · 2023-01-13T12:38:11Z

nixos/modules/hardware/gkraken.nix

+  inherit (lib)
+    mdDoc mkEnableOption mkIf;


Each of those functions is used once. The file is not even 20 lines long. Both the inherit and with should be dropped and the three occurrences of lib should just be written out.

Being redundant, my primary concern here is to make the code compile without with.

After this, it will be easy to prefix lib.

OTOH I don't believe this is bad code. It is similar to other programming languages like C++ namespaces and Python lib imports.

SuperSandro2000 · 2023-01-13T12:39:33Z

nixos/modules/testing/service-runner.nix

@@ -121,7 +122,7 @@ in
 {
  options = {
    systemd.services = mkOption {
-      type = with types; attrsOf (submodule opts);
+      type = attrsOf (submodule opts);


This usage of lib is totally fine for me and I see a lot of people doing it when writing modules. IMO we should keep it.

SuperSandro2000 · 2023-01-13T12:40:00Z

nixos/modules/testing/service-runner.nix

+    description = mdDoc ''
+      A script that runs the service outside of systemd, useful for testing or
+      for using NixOS services outside of NixOS.
    '';


We should not change formatting that changes the code to avoid unnecessary possibilities regressions.

SuperSandro2000 · 2023-01-13T12:46:07Z

nixos/tests/redis.nix

+      { pkgs, lib, ... }: let
+        inherit (lib) listToAttrs nameValuePair;
+      in


I suggest to either prefix the functions with lib. or move the with lib to the function.

SuperSandro2000 · 2023-01-13T12:47:09Z

nixos/tests/n8n.nix

+  meta.maintainers = builtins.attrValues {
+    inherit (lib.maintainers) freezeboy k900;
+  };


IMO this code is now actually worse and not understandable for a beginner. I am against changing multi maintainer entries to use a function instead of being a list which uses with very locally.

Beginners mostly copy and paste examples. Many of them still tops with lib; without knowing why - ans this is demonstrated by the use of lib.mdDoc.

SuperSandro2000 · 2023-01-13T12:47:32Z

nixos/tests/gnome-xorg.nix

-  meta = with lib; {
-    maintainers = teams.gnome.members;
-  };
+  meta.maintainers = lib.teams.gnome.members;


That's a good change 👍🏼

SuperSandro2000 · 2023-01-13T12:47:47Z

nixos/tests/gitlab.nix

-  meta = with pkgs.lib.maintainers; {
-    maintainers = [ globin yayayayaka ];
-  };
+  meta.maintainers = with lib.maintainers; [ globin yayayayaka ];


I would have done the same

SuperSandro2000 · 2023-01-13T12:47:58Z

nixos/tests/fluidd.nix

-with lib;
-
 {
  name = "fluidd";
-  meta.maintainers = with maintainers; [ vtuan10 ];
+  meta.maintainers = with lib.maintainers; [ vtuan10 ];


Great change 👍🏼

… `with lib;`

…ib;`

AndersonTorres · 2023-01-14T03:26:13Z

Small steps.

AndersonTorres · 2023-01-24T03:14:47Z

Closing because headache.

AndersonTorres marked this pull request as draft January 6, 2023 15:34

ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 labels Jan 6, 2023

thiagokokada reviewed Jan 8, 2023

View reviewed changes

SuperSandro2000 reviewed Jan 8, 2023

View reviewed changes

ajs124 mentioned this pull request Jan 11, 2023

qdmr: init at 0.11.2, added janik as maintainer #209733

Merged

13 tasks

github-actions bot removed the 6.topic: ocaml label Jan 12, 2023

SuperSandro2000 requested changes Jan 13, 2023

View reviewed changes

AndersonTorres added 7 commits January 14, 2023 00:13

nixos/maintainers/scripts/cloudstack/cloudstack-image.nix: get rid of…

2fba0f9

… `with lib;`

nixos/maintainers/scripts/lxd/lxd-image-inner.nix: get rid of `with l…

2f6eec7

…ib;`

nixos/maintainers/scripts/lxd/lxd-image.nix: get rid of with lib;

af67520

nixos/maintainers/scripts/lxd/nix.tpl: get rid of with lib;

8cce368

nixos/modules/testing/test-instrumentation.nix: get rid of with lib;

3101346

nixos/tests/hardened.nix: get rid of with lib;

76adca5

nixos/tests/nfs/kerberos.nix: get rid of with lib;

adadeea

github-actions bot removed 6.topic: emacs Text editor 6.topic: pantheon The Pantheon desktop environment 6.topic: cinnamon Desktop environment 6.topic: GNOME GNOME desktop environment and its underlying platform labels Jan 14, 2023

AndersonTorres requested a review from SuperSandro2000 January 14, 2023 03:25

AndersonTorres marked this pull request as ready for review January 14, 2023 03:26

AndersonTorres requested a review from joachifm as a code owner January 14, 2023 03:26

AndersonTorres requested a review from thiagokokada January 22, 2023 15:13

SuperSandro2000 marked this pull request as draft January 23, 2023 22:17

AndersonTorres closed this Jan 24, 2023

AndersonTorres deleted the with-lib-is-bad branch January 24, 2023 03:14

Janik-Haag mentioned this pull request Feb 2, 2023

anuko-time-tracker: init at 1.22.19.5806 #214063

Merged

16 tasks

Janik-Haag mentioned this pull request May 16, 2023

ios-webkit-debug-proxy: init at 1.9.0 #232212

Merged

12 tasks

philiptaron mentioned this pull request Mar 11, 2024

Remove top level with lib; and with import ... in pkgs/top-level/ #293835

Merged

13 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

treewide: get rid of `with lib;` and friends #209325

treewide: get rid of `with lib;` and friends #209325

AndersonTorres commented Jan 6, 2023 •

edited

Loading

AndersonTorres commented Jan 6, 2023

thiagokokada Jan 8, 2023

AndersonTorres Jan 8, 2023

thiagokokada Jan 8, 2023

thiagokokada Jan 8, 2023

AndersonTorres Jan 8, 2023

SuperSandro2000 Jan 13, 2023

SuperSandro2000 Jan 8, 2023

AndersonTorres Jan 9, 2023

SuperSandro2000 Jan 13, 2023

AndersonTorres Jan 13, 2023 •

edited

Loading

SuperSandro2000 Jan 23, 2023

AndersonTorres Jan 24, 2023

SuperSandro2000 left a comment

SuperSandro2000 Jan 13, 2023

SuperSandro2000 Jan 13, 2023

SuperSandro2000 Jan 13, 2023

AndersonTorres Jan 13, 2023

SuperSandro2000 Jan 13, 2023

SuperSandro2000 Jan 13, 2023

SuperSandro2000 Jan 13, 2023

SuperSandro2000 Jan 13, 2023

AndersonTorres Jan 14, 2023

SuperSandro2000 Jan 13, 2023

SuperSandro2000 Jan 13, 2023

SuperSandro2000 Jan 13, 2023

AndersonTorres commented Jan 14, 2023

AndersonTorres commented Jan 24, 2023

		inherit (lib)
		literalExpression mdDoc mkIf mkMerge mkOption mkRenamedOptionModule types;

treewide: get rid of with lib; and friends #209325

treewide: get rid of with lib; and friends #209325

Conversation

AndersonTorres commented Jan 6, 2023 • edited Loading

Description of changes

Things done

AndersonTorres commented Jan 6, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndersonTorres Jan 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SuperSandro2000 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndersonTorres commented Jan 14, 2023

AndersonTorres commented Jan 24, 2023

treewide: get rid of `with lib;` and friends #209325

treewide: get rid of `with lib;` and friends #209325

AndersonTorres commented Jan 6, 2023 •

edited

Loading

AndersonTorres Jan 13, 2023 •

edited

Loading