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

Better type deprecation messages #97114

Merged
merged 5 commits into from Sep 7, 2020
Merged

Conversation

@Infinisil
Copy link
Member

@Infinisil Infinisil commented Sep 4, 2020

Motivation for this change

We want to deprecate option types over time, but how it's currently done, it's very hard to figure out which options actually use these deprecated types. This PR improves on this by allowing types to set a deprecationMessage, which when set, is displayed along the option that uses the type.

So with an option using types.loaOf, you now get

trace: warning: The type `types.loaOf' of option `foo.bar' defined in
  `/home/infinisil/src/nixpkgs/config.nix' is deprecated. Mixing lists with
  attribute values is no longer possible; please use `types.attrsOf` instead. See
  https://github.com/NixOS/nixpkgs/issues/1800 for the motivation.

Similar for types.string and types.optionSet. I also removed types.list completely because it's been deprecated for 7 years.

Previous PR's/commits that deprecated option types: fd803fc, #54637, #66346 (#16750), #96042 (#1800, #63103, #77189)

I also plan to deprecate more types in the future.

Ping @roberth @rycee @Profpatsch @danbst @rnhmjoj @worldofpeace @cole-h

Things done
  • Checked that all the deprecated types are usable and display the deprecation message
@Infinisil Infinisil requested review from edolstra and nbp as code owners Sep 4, 2020
@Infinisil Infinisil force-pushed the Infinisil:type-deprecation branch from 5926a80 to 0ec301b Sep 4, 2020
@rnhmjoj
Copy link
Contributor

@rnhmjoj rnhmjoj commented Sep 4, 2020

👍 for removing list: it's been deprecated since I started using NixOS and only causes confusion.

I'm not sure about the message: loaOf and list have been completely removed can still be used string can still be used.

@Infinisil Infinisil mentioned this pull request Sep 4, 2020
2 of 2 tasks complete
@Infinisil
Copy link
Member Author

@Infinisil Infinisil commented Sep 4, 2020

I'm not sure about the message: loaOf and list have been completely removed can still be used string can still be used.

Not sure what you mean. loaOf is still there, it's just deprecated and aliased to attrsOf. string is also still there, it's also just deprecated and aliased to separatedString " ". Once they have been deprecated long enough we can remove them.

@roberth
roberth approved these changes Sep 4, 2020
@rnhmjoj
Copy link
Contributor

@rnhmjoj rnhmjoj commented Sep 4, 2020

@Infinisil I meant that the implementation of loaOf and list has been replaced with a change of semantics, while string continues to work exactly the same, except for the warning.

It was actually cole-h that noticed this in #96042

Even though types.loaOf is aliased to types.attrsOf now, I still think this counts as being removed

@Infinisil
Copy link
Member Author

@Infinisil Infinisil commented Sep 4, 2020

@rnhmjoj I guess that's just because only a subset of loaOf's functionality has been deprecated (the part about lists), while string has been deprecated because it has such a tempting name when it often isn't the right type. I don't think there's a problem with this.

Infinisil added 5 commits Sep 4, 2020
Previously the only way to deprecate a type was using

  theType = lib.warn "deprecated" (mkOptionType ...)

This caused the warning to be emitted when the type was evaluated, but
the error didn't include which option actually used that type.

With this commit, types can specify a deprecationMessage, which when
non-null, is printed along with the option that uses the type
Has been deprecated since fd803fc
(2013-08-22)
@Infinisil Infinisil force-pushed the Infinisil:type-deprecation branch from 0ec301b to a582f6a Sep 7, 2020
@Infinisil
Copy link
Member Author

@Infinisil Infinisil commented Sep 7, 2020

I updated it to also print the files where the option is defined.

@Infinisil Infinisil merged commit ed5a07c into NixOS:master Sep 7, 2020
16 checks passed
16 checks passed
tests tests
Details
action
Details
Evaluation Performance Report Evaluator Performance Report
Details
Wait for ofborg
Details
grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-darwin nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="a582f6a"; rev="a582f6adde8f4345582c80fc3ccfe0de3aa89480"; } ./pkgs/t
Details
grahamcofborg-eval-lib-tests nix-build --arg pkgs import ./. {} ./lib/tests/release.nix
Details
grahamcofborg-eval-nixos nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="a582f6a"; rev="a582f6adde8f4345582c80fc3ccfe0de3aa89480"; } ./nixos/
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="a582f6a"; rev="a582f6adde8f4345582c80fc3ccfe0de3aa89480"; } ./nixos/
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="a582f6a"; rev="a582f6adde8f4345582c80fc3ccfe0de3aa89480"; } ./nixos/
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="a582f6a"; rev="a582f6adde8f4345582c80fc3ccfe0de3aa89480"; } ./pkgs/t
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="a582f6a"; rev="a582f6adde8f4345582c80fc3ccfe0de3aa89480"; } ./pkgs/t
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="a582f6a"; rev="a582f6adde8f4345582c80fc3ccfe0de3aa89480"; } ./pkgs/t
Details
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details
@Infinisil Infinisil deleted the Infinisil:type-deprecation branch Sep 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.