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

Automatically document builtin constants #8330

Merged
merged 4 commits into from
Jun 27, 2023

Conversation

Ericson2314
Copy link
Member

@Ericson2314 Ericson2314 commented May 13, 2023

This is done in roughly the same way builtin functions are documented.

Also auto-link experimental features for primops, subsuming PR #8371.

Motivation

Keeping the documention with the code like this makes it much easier to ensure it stays up to date and in sync.

Context

Recent doc PRs alerted me to the fact that we didn't yet have this infrastructure.

I moved in the existing docs, and then wrote some rough placeholder sentences for the others.

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.

@github-actions github-actions bot added documentation new-cli Relating to the "nix" command repl The Read Eval Print Loop, "nix repl" command and debugger labels May 13, 2023
@Ericson2314 Ericson2314 removed new-cli Relating to the "nix" command repl The Read Eval Print Loop, "nix repl" command and debugger labels May 13, 2023
@fricklerhandwerk
Copy link
Contributor

While I certainly agree we should have this systematically and automatically documented, I'm not sure if we should actually expose null, true, false and the hidden constants.

In general, maybe it's better to leave things undocumented rather than having a non-informative one-liner until we have something sensible. If we wait until we collect information on all of them, the PR will become really large and possibly run into merge conflicts.

@Ericson2314
Copy link
Member Author

@fricklerhandwerk you can replace the strings will nullptr if you don't like them, and they won't be rendered. It is up to you.


I personally like the fact that even null, true, and false are important distinct values, they don't have special *syntax`: those are plain old identifiers that are used in the same way as other plain old identifiers.

@Ericson2314 Ericson2314 force-pushed the doc-auto-builtin-constants branch from a5a4c5e to 3d05484 Compare May 16, 2023 20:39
@github-actions github-actions bot added new-cli Relating to the "nix" command repl The Read Eval Print Loop, "nix repl" command and debugger labels May 16, 2023
@roberth
Copy link
Member

roberth commented May 18, 2023

Exposing __-prefixed constants is also not great. I think the recommendation is to

  1. Use Nixpkgs lib
  2. Use builtins. You can polyfill when builtins?foo == false. foo = builtins.foo or (fooReimpl)
  3. Nothing. I don't think __foo should be used.

src/libexpr/eval.cc Outdated Show resolved Hide resolved
@fricklerhandwerk
Copy link
Contributor

Triaged in Nix team meeting 2023-05-19:

  • @fricklerhandwerk: in favor of the infrastructure support
    • should not actually add any new documentation here, would blow up scope
  • @Ericson2314: just having something in the table of contents is better than nothing, even if the actual documentation contents are useless
  • @fricklerhandwerk: should merge it without documentation for now
  • assigned to @edolstra for review

@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/2023-05-19-nix-team-meeting-minutes-56/28446/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/2023-06-05-nix-team-meeting-minutes-60/28933/1

@Ericson2314 Ericson2314 force-pushed the doc-auto-builtin-constants branch from 3d05484 to ba7bd90 Compare June 16, 2023 12:29
src/libexpr/primops.cc Outdated Show resolved Hide resolved
src/libexpr/primops.cc Show resolved Hide resolved
src/libexpr/primops.cc Outdated Show resolved Hide resolved
src/libexpr/primops.cc Outdated Show resolved Hide resolved
doc/manual/generate-builtin-constants.nix Outdated Show resolved Hide resolved
src/libexpr/eval.hh Outdated Show resolved Hide resolved
src/libexpr/primops.cc Outdated Show resolved Hide resolved
src/libutil/experimental-features.hh Outdated Show resolved Hide resolved
src/libutil/json-utils.hh Outdated Show resolved Hide resolved
src/libexpr/eval.hh Outdated Show resolved Hide resolved
@Ericson2314 Ericson2314 force-pushed the doc-auto-builtin-constants branch 4 times, most recently from 46608c6 to ec96c6e Compare June 21, 2023 23:32
doc/manual/generate-builtins.nix Outdated Show resolved Hide resolved
doc/manual/generate-builtins.nix Outdated Show resolved Hide resolved
doc/manual/generate-builtins.nix Outdated Show resolved Hide resolved
src/libexpr/eval.hh Outdated Show resolved Hide resolved
src/libexpr/eval.hh Outdated Show resolved Hide resolved
src/libexpr/primops.cc Outdated Show resolved Hide resolved
src/libexpr/primops.cc Outdated Show resolved Hide resolved
src/libexpr/primops.cc Outdated Show resolved Hide resolved
src/libexpr/primops.cc Outdated Show resolved Hide resolved
src/libexpr/primops.cc Outdated Show resolved Hide resolved
@Ericson2314 Ericson2314 force-pushed the doc-auto-builtin-constants branch 4 times, most recently from 1fdcf36 to 7ecb072 Compare June 22, 2023 13:34
Copy link
Contributor

@fricklerhandwerk fricklerhandwerk left a comment

Choose a reason for hiding this comment

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

Sorry for stalling this. This PR is a huge improvement.

src/libexpr/primops.cc Outdated Show resolved Hide resolved
src/libexpr/primops.cc Outdated Show resolved Hide resolved
src/libexpr/primops.cc Outdated Show resolved Hide resolved
src/libexpr/primops.cc Outdated Show resolved Hide resolved
@Ericson2314 Ericson2314 force-pushed the doc-auto-builtin-constants branch from 5fb31db to 27081d4 Compare June 23, 2023 23:40
src/libexpr/primops.cc Outdated Show resolved Hide resolved
src/libexpr/primops.cc Outdated Show resolved Hide resolved
src/libexpr/primops.cc Outdated Show resolved Hide resolved
src/libexpr/primops.cc Outdated Show resolved Hide resolved
src/libexpr/primops.cc Outdated Show resolved Hide resolved
src/libexpr/primops.cc Outdated Show resolved Hide resolved
src/libexpr/primops.cc Outdated Show resolved Hide resolved
src/libexpr/primops.cc Outdated Show resolved Hide resolved
src/libexpr/primops.cc Outdated Show resolved Hide resolved
src/libexpr/primops.cc Outdated Show resolved Hide resolved
src/libexpr/primops.cc Outdated Show resolved Hide resolved
src/libexpr/primops.cc Outdated Show resolved Hide resolved
@Ericson2314 Ericson2314 force-pushed the doc-auto-builtin-constants branch from c812ebb to 99ee579 Compare June 27, 2023 13:11
@Ericson2314 Ericson2314 enabled auto-merge June 27, 2023 13:12
Any primop will do for this, so might as well use one that isn't impure.

Co-authored-by: Robert Hensing <[email protected]>
@Ericson2314 Ericson2314 force-pushed the doc-auto-builtin-constants branch from b5aaf8d to cca8b59 Compare June 27, 2023 13:35
Ericson2314 and others added 2 commits June 27, 2023 09:37
This is done in roughly the same way builtin functions are documented.

Also auto-link experimental features for primops, subsuming PR #8371.

Co-authored-by: Eelco Dolstra <[email protected]>
Co-authored-by: Robert Hensing <[email protected]>
Co-authored-by: Valentin Gagarin <[email protected]>
@Ericson2314 Ericson2314 force-pushed the doc-auto-builtin-constants branch from 7fa6140 to 22b278e Compare June 27, 2023 13:38
Copy link
Member

@roberth roberth left a comment

Choose a reason for hiding this comment

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

🚀

@Ericson2314 Ericson2314 merged commit 71d4fd8 into master Jun 27, 2023
@Ericson2314 Ericson2314 deleted the doc-auto-builtin-constants branch June 27, 2023 15:53
@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-50/29793/1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation new-cli Relating to the "nix" command
Projects
Status: Done
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants