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

Jump to bottom

Better type deprecation messages #97114

Merged
merged 5 commits into from Sep 7, 2020
Merged

Better type deprecation messages #97114

merged 5 commits into from Sep 7, 2020

Conversation

infinisil
Copy link
Member

@infinisil infinisil commented Sep 4, 2020 •
edited

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 added the 6.topic: module system About NixOS module system internals label Sep 4, 2020
@rnhmjoj
Copy link
Contributor

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
Merged
2 tasks
@infinisil
Copy link
Member Author

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.

@rnhmjoj
Copy link
Contributor

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 commented Sep 4, 2020 •
edited

@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
lib/types: Allow types to emit a deprecation warning
1d46562 
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
@infinisil
lib/types: Remove types.list, it's been deprecated long enough
14095f8 
Has been deprecated since fd803fc
(2013-08-22)
@infinisil
lib/types: Set deprecationMessage for types.string
2bed3b2
@infinisil
lib/types: Set deprecationMessage for types.loaOf
3b7aca4
@infinisil
lib/types: Set deprecationMessage for types.optionSet
a582f6a
@infinisil
Copy link
Member Author

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

@infinisil infinisil merged commit ed5a07c into NixOS:master Sep 7, 2020
@infinisil infinisil deleted the type-deprecation branch September 7, 2020 17:33
This was referenced Sep 27, 2020
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@roberth roberth roberth approved these changes

@edolstra edolstra Awaiting requested review from edolstra

@nbp nbp Awaiting requested review from nbp

Assignees
No one assigned
Labels
6.topic: module system About NixOS module system internals 10.rebuild-darwin: 0 10.rebuild-linux: 0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@infinisil @rnhmjoj @roberth