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

Properly deprecate types.string #66346

Merged
merged 2 commits into from
Aug 31, 2019
Merged

Properly deprecate types.string #66346

merged 2 commits into from
Aug 31, 2019

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 requested review from edolstra and nbp as code owners August 8, 2019 21:26
@infinisil infinisil changed the title Remove types.string Properly deprecate types.string Aug 8, 2019
@ofborg ofborg bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux labels 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 🙂

@infinisil infinisil mentioned this pull request Aug 9, 2019
Merged
10 tasks
This was referenced Sep 6, 2019
Closed
Closed
@pingiun pingiun mentioned this pull request Oct 24, 2019
Open
dtzWill pushed a commit to dtzWill/nixpkgs that referenced this pull request Jan 19, 2020
@ivan @dtzWill
nixos/railcar: remove use of the deprecated string type
a8eb967 
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
Merged
1 task
@celestefox celestefox mentioned this pull request Sep 12, 2020
Closed
allgreed added a commit to allgreed/nixops that referenced this pull request Sep 13, 2020
@allgreed
Deprecate types.string
72d1390 
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
@infinisil
lib/modules: Make sure to not import module _file's into the store
769eac0 
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.
@infinisil infinisil mentioned this pull request Mar 1, 2022
Merged
13 tasks
github-actions bot pushed a commit to nix-community/nixpkgs.lib that referenced this pull request Dec 4, 2022
@infinisil
lib/modules: Make sure to not import module _file's into the store
78eba1a 
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 jian-lin mentioned this pull request Aug 8, 2023
Merged
12 tasks
jalil-salame added a commit to jalil-salame/NixNeovim that referenced this pull request Aug 27, 2023
@jalil-salame
fix(nix): types.string deprecated
15bf0a8 
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.
@jalil-salame jalil-salame mentioned this pull request Aug 27, 2023
Merged
@name-snrl name-snrl mentioned this pull request Aug 30, 2023
Merged
12 tasks
@Whovian9369 Whovian9369 mentioned this pull request Nov 30, 2023
Closed
@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
@slotThe
flake: Use types.str instead of types.string
5cf87c7 
types.string is deprecated.

Related: NixOS/nixpkgs#66346
gwarf added a commit to gwarf/dotfiles that referenced this pull request Mar 12, 2024
@gwarf
Fix types.string deprecation
f18196c 
NixOS/nixpkgs#66346
shivaraj-bh added a commit to juspay/services-flake that referenced this pull request Mar 29, 2024
@shivaraj-bh
refactor(deprecation): replace types.string with types.str
423b854 
see NixOS/nixpkgs#66346
@mbssrc mbssrc mentioned this pull request Jul 9, 2024
Merged
ackttic pushed a commit to gwarf/dotfiles that referenced this pull request Oct 19, 2024
@gwarf
Fix types.string deprecation
95ff017 
NixOS/nixpkgs#66346
alexnguyennn added a commit to alexnguyennn/nix-kmonad that referenced this pull request Nov 10, 2024
@alexnguyennn
chore: bump config type to something not deprecated
97c2083 
* NixOS/nixpkgs#66346
jules-sommer added a commit to jules-sommer/nix_config_minimal that referenced this pull request Nov 30, 2024
@jules-sommer
[fix/chore]: remove all instances of deprecated types.string
af5435e 
- as per NixOS/nixpkgs#66346
- also disabled gnome-keyring to use keepassxc secret agent instead
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@danbst danbst danbst left review comments

@rycee rycee rycee approved these changes

@edolstra edolstra Awaiting requested review from edolstra

@nbp nbp Awaiting requested review from nbp

@peti peti Awaiting requested review from peti

Assignees
No one assigned
Labels
6.topic: module system About "NixOS" module system internals 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@infinisil @rycee @Profpatsch @danbst @pacien @wkordalski @Mic92