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

nixos/bat: fix settings type handling #392495

Merged
merged 1 commit into from
Mar 24, 2025
Merged

nixos/bat: fix settings type handling #392495

merged 1 commit into from
Mar 24, 2025

Conversation

tanya1866
Copy link
Contributor

Converts configuration values recursively using toString instead of manual quoting while maintaining automatic space-handling through the keyValue generator. This supports:

  • Native number types (0, 3.14)
  • Booleans (true/false)
  • Nested lists with mixed types
  • Existing string behavior with spaces

Closes #392323
Maintainer: @SigmaSquadron

@github-actions github-actions 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/` labels Mar 23, 2025
@JohnRTitor JohnRTitor changed the title programs.bat: fix settings type handling nixos/bat: fix settings type handling Mar 23, 2025
@tanya1866 tanya1866 force-pushed the bat-fix-settings-type branch from b39cfcb to 5fb3b23 Compare March 23, 2025 19:56
JohnRTitor
JohnRTitor reviewed Mar 23, 2025
View reviewed changes
Copy link
Contributor

@JohnRTitor JohnRTitor 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.

Hi, welcome to Nixpkgs!

The commit title should be as the PR title (just like I did), please amend and force push.

@tanya1866
Copy link
Contributor Author

tanya1866 commented Mar 23, 2025 •
edited
Loading

hii, thank you
okay

@tanya1866 tanya1866 force-pushed the bat-fix-settings-type branch from 5fb3b23 to ee299a5 Compare March 23, 2025 19:59
SigmaSquadron
SigmaSquadron reviewed Mar 23, 2025
View reviewed changes
Copy link
Contributor

@SigmaSquadron SigmaSquadron left a comment

Choose a reason for hiding this comment

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

Seems good to me! Just a small stylistic nit to fit the rest of the module.

nixos/modules/programs/bat.nix Outdated
recursiveToString = value:
if isList value then
map recursiveToString value
else if builtins.isBool value then
Copy link
Contributor

@SigmaSquadron SigmaSquadron Mar 23, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Suggested change
else if builtins.isBool value then
else if isBool value then

Inherit isBool from lib. This is purely stylistic, as we avoid having to call for builtins.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure

Copy link
Contributor

@SigmaSquadron SigmaSquadron Mar 23, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

You also need to add it to the inherit statement at the top of the file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@github-actions github-actions bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 labels Mar 23, 2025
@tanya1866 tanya1866 force-pushed the bat-fix-settings-type branch 3 times, most recently from 11fff86 to f457dc4 Compare March 23, 2025 20:12
eclairevoyant
eclairevoyant reviewed Mar 23, 2025
View reviewed changes
nixos/modules/programs/bat.nix Outdated
if isList value then
map recursiveToString value
else if isBool value then
if value then "true" else "false"

This comment was marked as resolved.

Sign in to view

nixos/modules/programs/bat.nix
@@ -18,6 +18,7 @@ let
nameValuePair
optionalString
types
isBool

This comment was marked as resolved.

Sign in to view

eclairevoyant
eclairevoyant reviewed Mar 23, 2025
View reviewed changes
Copy link
Contributor

@eclairevoyant eclairevoyant left a comment

Choose a reason for hiding this comment

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

Also, this PR removes enclosing quotes from string values, is that desirable?

@tanya1866 tanya1866 force-pushed the bat-fix-settings-type branch from f457dc4 to 010f3ea Compare March 24, 2025 06:04
@tanya1866
Copy link
Contributor Author

tanya1866 commented Mar 24, 2025 •
edited
Loading

okay i have changed it
and the quote removal was intentional - pkgs.formats.keyValue automatically adds quotes when needed (for values with spaces)

@tanya1866 tanya1866 requested a review from eclairevoyant March 24, 2025 06:08
@tanya1866
Copy link
Contributor Author

tanya1866 commented Mar 24, 2025 •
edited
Loading

is it okay now? @eclairevoyant @SigmaSquadron

@SigmaSquadron
Copy link
Contributor

I don't know about removing the quotes. Does Bat still parse everything correctly?

@eclairevoyant
Copy link
Contributor

@SigmaSquadron I asked the same, evidently this handles it:

nixpkgs/nixos/modules/programs/bat.nix

Line 28 in 010f3ea

settingsFormat = pkgs.formats.keyValue { listsAsDuplicateKeys = true; };

I haven't had a chance to test it yet, and I didn't see any tests for the module, perhaps adding some in a future PR would be ideal.

@SigmaSquadron
Copy link
Contributor

I'll look into adding tests ~eventually. Shouldn't be much different from other CLI tool tests.

SigmaSquadron
SigmaSquadron approved these changes Mar 24, 2025
View reviewed changes
Copy link
Contributor

@SigmaSquadron SigmaSquadron left a comment

Choose a reason for hiding this comment

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

Manually tested; it does seem to work just fine without quotes. LGTM.

@JohnRTitor JohnRTitor merged commit 5c72fd6 into NixOS:master Mar 24, 2025
28 of 29 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SigmaSquadron SigmaSquadron

@JohnRTitor JohnRTitor

@eclairevoyant eclairevoyant

Assignees
No one assigned
Labels
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: 1-10
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

nixos/bat: settings keys only accept string values, despite the format accepting bools and numbers as well
4 participants
@tanya1866 @SigmaSquadron @eclairevoyant @JohnRTitor