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

Mark experimental configuration settings programmatically #8196

Merged
merged 2 commits into from Apr 17, 2023

Conversation

Ericson2314
Copy link
Member

@Ericson2314 Ericson2314 commented Apr 10, 2023

Motivation

Fix #8162

Context

The test is changed to compare nlohmann::json values, not strings of dumped JSON, which allows us to format things more nicely.

This is somewhat hard to observe until #8174 is merged, fixing the accidental hiding of experimental features that aren't currently enabled, but the fix is technically orthogonal.

No longer true. The #8201 stop-gap also makes it show up, and has been merged.

Checklist for maintainers

Maintainers: tick if completed or explain if not relevant

  • agreed on idea
  • agreed on implementation strategy
  • tests, as appropriate
    • functional tests - tests/**.sh
    • unit tests - src/*/tests
    • integration tests - tests/nixos/*
  • documentation in the manual
  • documentation in the internal API docs
  • code and comments are self-explanatory
  • commit message explains why the change was made
  • new feature or incompatible change: updated release notes

Priorities

Add 👍 to pull requests you find important.

@Ericson2314 Ericson2314 force-pushed the fix-8162 branch 4 times, most recently from 09829cb to 9d58840 Compare April 15, 2023 15:18
@github-actions github-actions bot added the with-tests Issues related to testing. PRs with tests have some priority label Apr 15, 2023
@Ericson2314
Copy link
Member Author

OK Fixed the unit tests.

src/libutil/config.cc Outdated Show resolved Hide resolved
Comment on lines 60 to 65
> **Warning**
> This setting is part of an experimental feature.

To change this setting, you need to make sure the corresponding experimental feature,
[`${experimental-feature}`](@docroot@/contributing/experimental-features.md#xp-feature-${experimental-feature}),
is enabled.
For example, include the following in [`nix.conf`](#):
Copy link
Member

Choose a reason for hiding this comment

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

We should explain when to use it and when not to use it, probably by linking the a page about experimental features?

Copy link
Member Author

Choose a reason for hiding this comment

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

The section on the specific experimental feature is the same page, but also I made the warning above link the page as a whole (not URL fragment).

@@ -56,6 +56,21 @@ rec {
body = ''
${description}

'' + (if experimental-feature != null then ''
Copy link
Member

Choose a reason for hiding this comment

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

I feel like string interpolation looks cleaner, but feel free to ignore.
In Nixpkgs I would also say lib.optionalString, but that may not apply here.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with that, and recommend following my example of creating helper functions with self-explanatory names when inline expressions get out of hand.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added optionalString in a preparatory commit, and used it in a bunch of other places too.

@Ericson2314 Ericson2314 force-pushed the fix-8162 branch 2 times, most recently from 722199b to 6db3dcb Compare April 16, 2023 14:57
Use it everywhere it could be also.
Fix NixOS#8162

The test is changed to compare `nlohmann::json` values, not strings of dumped
JSON, which allows us to format things more nicely.
> **Warning**
> This is an experimental feature.

To enable it, add the following to [`nix.conf`](#):
Copy link
Member

Choose a reason for hiding this comment

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

@roberth roberth merged commit 36a473c into NixOS:master Apr 17, 2023
8 checks passed
@Ericson2314 Ericson2314 deleted the fix-8162 branch April 17, 2023 10:36
@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/tweag-nix-dev-update-47/27387/1

@nixos-discourse
Copy link

This issue has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/tweag-nix-dev-update-47/27387/1

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/this-month-in-nix-docs-2-april-2023/27899/1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation with-tests Issues related to testing. PRs with tests have some priority
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Mark experimental configuration settings programmatically
4 participants