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.

Sign up for GitHub

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

Jump to bottom

treewide: deprecate isNull #219747

Merged
merged 1 commit into from
Mar 16, 2023
Merged

treewide: deprecate isNull #219747

merged 1 commit into from
Mar 16, 2023

Conversation

Stunkymonkey
Copy link
Contributor

@Stunkymonkey Stunkymonkey commented Mar 5, 2023 •
edited
Loading

Description of changes

https://nixos.org/manual/nix/stable/language/builtins.html#builtins-isNull

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 23.05 Release Notes (or backporting 22.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

pennae
pennae reviewed Mar 5, 2023
View reviewed changes
Copy link
Contributor

@pennae pennae left a comment

Choose a reason for hiding this comment

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

why though? it's not hurting anyone?

pkgs/applications/editors/emacs/elisp-packages/libgenerated.nix Show resolved Hide resolved
@Stunkymonkey
Copy link
Contributor Author

why though? it's not hurting anyone?

@pennae it is mentioned here to be deprecated: https://nixos.org/manual/nix/stable/language/builtins.html#builtins-isNull

@bjornfor
Copy link
Contributor

bjornfor commented Mar 6, 2023

why though? it's not hurting anyone?

@pennae it is mentioned here to be deprecated: https://nixos.org/manual/nix/stable/language/builtins.html#builtins-isNull

Then please write that in the commit message.

@sternenseemann
Copy link
Member

It may be arbitrarily “deprecated”, but that quite frankly doesn't really mean anything in the case of Nix. It's not like it's going to be removed.

@Stunkymonkey
Copy link
Contributor Author

would you prefer having this split up into multiple PR? or is it not worth it?

@pennae
Copy link
Contributor

pennae commented Mar 11, 2023

dunno. it just doesn't seem worth doing, to be honest? the review effort is so much higher than the benefits (which are entirely aesthetic)

SuperSandro2000
SuperSandro2000 approved these changes Mar 12, 2023
View reviewed changes
Copy link
Member

@SuperSandro2000 SuperSandro2000 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I carefully looked at everything and did not notice anything that would change the code outcome. nixos-install-tools is rebuild everytime lib changes even if the code is identical.

@SuperSandro2000
Copy link
Member

At least !isNull var is for myself a bit harder to process than var != null and I think we should not encourage that pattern.

@lovesegfault lovesegfault merged commit 6e55733 into NixOS:master Mar 16, 2023
@infinisil infinisil mentioned this pull request Apr 26, 2023
Closed
@Stunkymonkey Stunkymonkey deleted the deprecate-isNull branch November 22, 2023 21:36
@SuperSandro2000 SuperSandro2000 mentioned this pull request Dec 20, 2023
Closed
13 tasks
infinisil added a commit to tweag/nix that referenced this pull request Dec 20, 2023
@infinisil
Undeprecate isNull
9d6a80b 
There's no good reason to deprecate it:
- For consistency reasons it should continue to exist, such that all
primitive types have a corresponding `builtins.is*` primop.
- There's no implementation cost to continuing to have this function
- It costs users time to try to migrate away from it, e.g.
  NixOS/nixpkgs#219747 and NixOS/nixpkgs#275548
- Using it can give easier-to-read code like `all isNull list`
@infinisil infinisil mentioned this pull request Dec 20, 2023
Merged
infinisil added a commit to tweag/nix that referenced this pull request Dec 20, 2023
@infinisil @roberth
Undeprecate isNull
ea454d8 
There's no good reason to deprecate it:
- For consistency reasons it should continue to exist, such that all
primitive types have a corresponding `builtins.is*` primop.
- There's no implementation cost to continuing to have this function
- It costs users time to try to migrate away from it, e.g.
  NixOS/nixpkgs#219747 and NixOS/nixpkgs#275548
- Using it can give easier-to-read code like `all isNull list`

Co-authored-by: Robert Hensing <roberth@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pennae pennae pennae left review comments

@SuperSandro2000 SuperSandro2000 SuperSandro2000 approved these changes

@wegank wegank wegank approved these changes

@zowoq zowoq Awaiting requested review from zowoq

@ehmry ehmry Awaiting requested review from ehmry ehmry is a code owner

@adisbladis adisbladis Awaiting requested review from adisbladis adisbladis is a code owner

@cdepillabout cdepillabout Awaiting requested review from cdepillabout

@sternenseemann sternenseemann Awaiting requested review from sternenseemann sternenseemann is a code owner

@maralorn maralorn Awaiting requested review from maralorn maralorn is a code owner

@Ericson2314 Ericson2314 Awaiting requested review from Ericson2314 Ericson2314 is a code owner

@matthewbauer matthewbauer Awaiting requested review from matthewbauer

@edolstra edolstra Awaiting requested review from edolstra

@Profpatsch Profpatsch Awaiting requested review from Profpatsch Profpatsch is a code owner

Assignees
No one assigned
Labels
6.topic: emacs 6.topic: haskell 6.topic: nim Nim programing language 6.topic: nixos 6.topic: python 6.topic: stdenv Standard environment 8.has: module (update) 10.rebuild-darwin: 0 10.rebuild-linux: 1-10
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@Stunkymonkey @bjornfor @sternenseemann @pennae @SuperSandro2000 @wegank @lovesegfault