Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

lib/tests: remove top-level with in lib/tests #295020

Merged
merged 3 commits into from Mar 11, 2024

Conversation

philiptaron
Copy link
Contributor

Description of changes

Using with at a top-level scope is an anti-pattern. The tracking issue is #208242. This is a pure refactor: there is no functional change contained in this PR.

I used the refactor strategy that looks for the uses of names coming from lib.

This PR changes the files that used top-level with lib; and with import ...; in the lib/tests/ directory.

Things done

  • Tested, as applicable:
    • nix-instantiate --strict --eval lib/tests/misc.nix
    • lib/tests/modules.sh
  • Fits CONTRIBUTING.md.

@github-actions github-actions bot added 6.topic: module system About NixOS module system internals 6.topic: lib The Nixpkgs function library labels Mar 11, 2024
@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/removing-most-uses-of-top-level-with/41233/1

Comment on lines -44 to +51
enableAlias = lib.mkForce false;
enableAlias = mkForce false;
Copy link
Contributor

Choose a reason for hiding this comment

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

The lib here is from the set pattern right above, not from the top-level, so should this perhaps remain unchanged?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it's clearer, since instead of two ways of referring to the same symbol, there's just one.

And, it turns out, it's also more performant. That doesn't matter in this file, but as a practice, it's good, in my judgement.

Copy link
Member

Choose a reason for hiding this comment

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

In general, this would only be relevant when combining two versions of the module system.
On top of that, this is just a test, with no other version of lib in sight.

@roberth roberth merged commit e8d7a2d into NixOS:master Mar 11, 2024
24 checks passed
@philiptaron philiptaron deleted the remove-top-level-with-in-lib-part4 branch March 11, 2024 20:39
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.

None yet

4 participants