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

Introduce types.optionType and use it for freeformType #149689

Merged
merged 2 commits into from
Mar 2, 2022
Merged

Introduce types.optionType and use it for freeformType #149689

merged 2 commits into from
Mar 2, 2022

Conversation

infinisil
Copy link
Member

@infinisil infinisil commented Dec 8, 2021 •
edited
Loading

Motivation for this change

By introducing types.optionType and using it for freeformType, we can fix #132085, making sure that the declaration location of submodules in freeformTypes are shown in the manual, because this new type properly propagates file locations.

In addition however, this also allows declaring multiple freeformTypes using the normal type merging behavior which can merge submodules together.

This also removes awkwardness regarding what type the option freeformType should be.

Ping @ncfavier, thanks for the issue!
Ping @roberth for review

Things done
  • Built the NixOS manual using nix-build nixos/release.nix -A manualHTML.x86_64-linux and verified that the option services.wordpress.<name>.package newly shows where it was declared.
  • Added tests for:
    • freeformType submodules having declaration locations
    • Being able to specify multiple freeformTypes and them being merged together

@infinisil infinisil added the 6.topic: module system About "NixOS" module system internals label Dec 8, 2021
@infinisil infinisil mentioned this pull request Dec 8, 2021
Closed
@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: documentation This PR adds or changes documentation labels Dec 8, 2021
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 labels Dec 8, 2021
@domenkozar
Copy link
Member

Does it make types.unspecified obsolete?

@infinisil
Copy link
Member Author

@domenkozar Not this PR, but #97119 and #132448 do! Same for types.attrs

roberth
roberth reviewed Dec 11, 2021
View reviewed changes
lib/types.nix Outdated
type = mkOptionType {
name = "type";
description = "type";
check = isAttrs;
Copy link
Member

Choose a reason for hiding this comment

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

This should check at least _type = "option-type";

Naming isn't quite consistent: mkOptionType/"option-type"/types.type. "Option type" currently wins, but I do think it's a bit verbose. Renaming mkOptionType -> mkType would also be ok. Just don't deprecate immediately.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it's fine to be a bit on the verbose side, especially also because types.type is kind of weird to see, aligning that with mkOptionType should make it a bit more obvious. I'll improve the check and change the name to types.optionType

Copy link
Member Author

Choose a reason for hiding this comment

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

Also in the module system there's other "types" than just option types, see the other values that _type can have. So I think naming this types.optionType is very fitting.

@infinisil infinisil requested review from Ma27 and roberth February 17, 2022 20:33
@ofborg ofborg bot added the 2.status: merge conflict This PR has merge conflicts with the target branch label Feb 23, 2022
@infinisil infinisil changed the title Introduce types.type and use it for freeformType Introduce types.optionType and use it for freeformType Feb 28, 2022
@infinisil
Copy link
Member Author

infinisil commented Feb 28, 2022 •
edited
Loading

Addressed your reviews, @Ma27 and @roberth, and just rebased to fix conflicts. Would love to merge this soon!

@ofborg ofborg bot removed the 2.status: merge conflict This PR has merge conflicts with the target branch label Feb 28, 2022
roberth
roberth reviewed Mar 1, 2022
View reviewed changes
lib/types.nix Outdated
optionType = mkOptionType {
name = "optionType";
description = "optionType";
check = value: value ? _type && value._type == "option-type";
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
check = value: value ? _type && value._type == "option-type";
check = value: value._type or null == "option-type";

slightly simpler

Copy link
Member Author

Choose a reason for hiding this comment

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

Applied

roberth
roberth reviewed Mar 1, 2022
View reviewed changes
infinisil added 2 commits March 1, 2022 19:31
@infinisil
lib.types: Introduce types.optionType
5cbeddf 
This type correctly merges multiple option types together while also
annotating them with file information. In a future commit this will be
used for `_module.freeformType`
@infinisil
lib.modules: Use types.optionType for _module.freeformType
023fa7b 
This ensures that the module file locations are propagated to the
freeform type, which makes it so that submodules in freeform types now
have their declaration location shown in the manual, fixing
NixOS#132085.

In addition, this also newly allows freeformTypes to be declared
multiple times and all declarations being merged together according to
normal option merging.

This also removes some awkwardness regarding the type of `freeformType`
@infinisil infinisil merged commit c1dfec2 into NixOS:master Mar 2, 2022
@infinisil infinisil deleted the types-type branch March 2, 2022 17:29
@roberth
Copy link
Member

roberth commented Mar 10, 2022

Small regression: #163597

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@roberth roberth roberth approved these changes

@edolstra edolstra Awaiting requested review from edolstra

@nbp nbp Awaiting requested review from nbp

@Ma27 Ma27 Awaiting requested review from Ma27

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: documentation This PR adds or changes documentation 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.

Options declared under a freeformType don't have declaration locations
4 participants
@infinisil @domenkozar @roberth @Ma27