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

Properly deprecate types.string #66346

Merged
merged 2 commits into from Aug 31, 2019
Merged

Conversation

infinisil
Copy link
Member

Motivation for this change

For almost 6 (!) years now, types.string has been marked as deprecated in the comment above its definition. People keep using this type regardless, and it's getting annoying while reviewing PRs to constantly have to remind people to not use it.

This PR does following things:

  • Remove all usages of types.string in nixpkgs and replace them with a more appropriate type
  • Make types.string emit a warning upon use

The mass replacement was done all manually, because I needed to decide what the replacement type should be. This was either

  • types.str when merging doesn't make sense (e.g. user options)
  • types.separatedString " " if its an option representing command line arguments (ideally it should be listOf str for new modules)
  • types.lines if it's a line-based configuration format (where joining with newlines makes sense)
  • types.str if it's a different configuration format where concatenation doesn't work

Ping @Profpatsch @rycee @aanderse @danbst

Things done

I have not tested any of the modules. While there's a very low chance of any of these changes breaking anything, I might have made a mistake somewhere. A double-check would therefore be good.

  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)

@infinisil infinisil changed the title Remove types.string Properly deprecate types.string Aug 8, 2019
@rycee
Copy link
Member

rycee commented Aug 8, 2019

Wonderful! I've recently been thinking about doing this exact thing since I see people accidentally using types.string in new modules all the time. It's a waste of energy to always have to look for and point this out when reviewing.

I'll make a proper review on the 12th 🙂

@pingiun pingiun mentioned this pull request Oct 24, 2019
dtzWill pushed a commit to dtzWill/nixpkgs that referenced this pull request Jan 19, 2020
This fixes the warning being emitted by nixos-rebuild switch:

building Nix...
building the system configuration...
trace: warning: types.string is deprecated because it quietly concatenates strings

It started emitting a warning in NixOS#66346.

(cherry picked from commit 5a03f90)
@infinisil infinisil added the 6.topic: module system About NixOS module system internals label Mar 19, 2020
@infinisil infinisil mentioned this pull request Sep 4, 2020
1 task
allgreed added a commit to allgreed/nixops that referenced this pull request Sep 13, 2020
As per NixOS/nixpkgs#66346 suggestion I'm going
for 'str' since it's a list of arguments and is only further processed
by Python
infinisil added a commit to infinisil/nixpkgs that referenced this pull request Sep 29, 2020
Previously if `_file` was specified by a module:
  trace: warning: The type `types.string' of option `foo' defined in `/nix/store/yxhm2il5yrb92fldgriw0wyqh2kk9qyc-bug.nix' is deprecated. See NixOS#66346 for better alternative types.

With this change:
  trace: warning: The type `types.string' of option `foo' defined in `/home/infinisil/src/nixpkgs/bug.nix' is deprecated. See NixOS#66346 for better alternative types.
github-actions bot pushed a commit to nix-community/nixpkgs.lib that referenced this pull request Dec 4, 2022
Previously if `_file` was specified by a module:
  trace: warning: The type `types.string' of option `foo' defined in `/nix/store/yxhm2il5yrb92fldgriw0wyqh2kk9qyc-bug.nix' is deprecated. See NixOS/nixpkgs#66346 for better alternative types.

With this change:
  trace: warning: The type `types.string' of option `foo' defined in `/home/infinisil/src/nixpkgs/bug.nix' is deprecated. See NixOS/nixpkgs#66346 for better alternative types.
@infinisil infinisil added the significant Novel ideas, large API changes, notable refactorings, issues with RFC potential, etc. label Apr 19, 2023
@infinisil infinisil removed the significant Novel ideas, large API changes, notable refactorings, issues with RFC potential, etc. label Aug 8, 2023
jian-lin added a commit to linj-fork/nixpkgs that referenced this pull request Aug 8, 2023
It is an error[1] now.

[1]: NixOS#66346
@jian-lin jian-lin mentioned this pull request Aug 8, 2023
12 tasks
jalil-salame added a commit to jalil-salame/NixNeovim that referenced this pull request Aug 27, 2023
see: NixOS/nixpkgs#66346

Use `types.str` when merging makes no sense (ie. "Hello" "World"
shouln't become "HelloWorld"), `types.separatedString "${sep}"` when
merging should be done (prefer `listOf str` though), and `types.lines`
for line based configuration.
@wkordalski
Copy link
Contributor

Even now https://nixos.wiki/wiki/Declaration informs that types.string is the right thing to use.
I don't know if this site is yours or not, but for sure a significant number of NixOS beginners learns there how to use mkOption.
Putting correct information there might help with the repeating types.string.

@Mic92
Copy link
Member

Mic92 commented Dec 14, 2023

https://nixos.wiki/wiki/Declaration

Everyone can edit their, including you. I fixed it now.

slotThe added a commit to xmonad/xmonad that referenced this pull request Feb 8, 2024
gwarf added a commit to gwarf/dotfiles that referenced this pull request Mar 12, 2024
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

7 participants