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

doc: migrate lib.attrsets to use doc-comments #296186

Merged
merged 3 commits into from
Mar 20, 2024

Conversation

hsjobeki
Copy link
Contributor

@hsjobeki hsjobeki commented Mar 15, 2024

Description of changes

Doc-comment migration

cc @DanielSidhion

I am going to manually change the following after a the first round of review is done.

  • Inner/outer format migrated /* {nixdoc} */ -> /** {commonmark} */
  • Clean up function arguments. Manually remove the redundant line comments. We decided the codemod won't delete anything.
  • Reify examples. After the migration is done all examples should be placed at the end under an # Examples heading.
    • Examples should be surrunded by :::{.example} such that they are properly styled.
    • Make sure examples have a descriptive heading. e.g. ## lib.asserts.assertMsg usage examples
  • Manual reformatting work for consistency:
    • foldlAttrs: Attention block needs proper highlighting
    • Indentation: getAttrFromPath,
    • Whitespace: attrsToList
    • mapAttrsRecursive / mapAttrsRecursiveCond: inconsistent order of example / type. (Wont fix)

Please comment all other errors / mistakes.

NOTE: PRs should not add/change content (excluding minor fixes) so we keep track of eventual rendering problems arising with the migration in isolation.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.05 Release Notes (or backporting 23.05 and 23.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@github-actions github-actions bot added the 6.topic: lib The Nixpkgs function library label Mar 15, 2024
@hsjobeki
Copy link
Contributor Author

@infinisil @DanielSidhion I think this PR should be ready to merge. If you'd give it a rough review.

I need to fix this in nixdoc: nix-community/nixdoc#113 but shouldn't be a blocker.

Copy link
Member

@DanielSidhion DanielSidhion left a comment

Choose a reason for hiding this comment

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

Looks good to me! No information lost and the formatting is great.

While reading through the changes I made a few suggestions of content to add (since the effort to write the suggestions was small and we're changing this stuff already).

lib/attrsets.nix Show resolved Hide resolved
lib/attrsets.nix Show resolved Hide resolved
lib/attrsets.nix Show resolved Hide resolved
lib/attrsets.nix Show resolved Hide resolved
lib/attrsets.nix Show resolved Hide resolved
lib/attrsets.nix Show resolved Hide resolved
lib/attrsets.nix Show resolved Hide resolved
lib/attrsets.nix Show resolved Hide resolved
@DanielSidhion DanielSidhion merged commit a737a78 into NixOS:master Mar 20, 2024
20 checks passed
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.

2 participants