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

lib/options: introduce "prime" fn variants (alt API) #1603

Merged
merged 12 commits into from
Jun 1, 2024

Conversation

MattSturgeon
Copy link
Member

@MattSturgeon MattSturgeon commented May 31, 2024

This PR adds new variants for all low-lever option factory funcs (mkNullable, mkNullOrOption, etc), as well as a handful of higher-level factories (mkAttributeSet, mkAttrsOf, & mkListOf).

This is done so that we have easy access to the underlying lib.mkOption attributes, such as example.

All attributes are passed through blindly, unless they are intentionally modified along the way.

Currently this PR doesn't make any effort to document required arg-attributes.

I made (more or less) every function its own commit because I needed to use git bisect.
This will also make it easier to drop/revert any specific function if we decide it's overkill to have a prime variant of it.
I'll happily squash once we're ready to merge - how would you like the history structured?

Unrelated change: combined mkDesc and mkDefaultDesc into a single function, since mkDefaultDesc was not really intended to be used directly. The same functionality can be achieved by passing an empty description to mkDesc.

@MattSturgeon MattSturgeon force-pushed the default_nulls branch 5 times, most recently from 0b363ac to 3a4701d Compare May 31, 2024 18:56
@MattSturgeon MattSturgeon requested a review from a team May 31, 2024 19:05
@GaetanLepage
Copy link
Member

I'll happily squash once we're ready to merge - how would you like the history structured?

I don't think that a squash is necessary. I am fine with one commit per function.

Unrelated change: combined mkDesc and mkDefaultDesc into a single function, since mkDefaultDesc was not really intended to be used directly. The same functionality can be achieved by passing an empty description to mkDesc.

Makes sense indeed.

@GaetanLepage
Copy link
Member

My opinion on this quite mixed.
On the one hand, I find it quite appealing to get those new helpful functions.
At the same time, this "bloats" (maybe a bit too strong of a word) our already quite heavy helpers function set. I am afraid that this could "scare" even more potential new contributors. But this is just a feeling.

So, no strong reject from my side. I would be curious to see what @traxys thinks about that.

@MattSturgeon
Copy link
Member Author

At the same time, this "bloats" (maybe a bit too strong of a word) our already quite heavy helpers function set. I am afraid that this could "scare" even more potential new contributors. But this is just a feeling.

In principal, I don't think we should be afraid of growing helpers. Although if it does grow larger, we might consider putting less stuff at the top-level (same principal as NixOS/nixpkgs#266103). If we avoid adding functions for this reason alone, we will end up making our lives more difficult in the long-run.

I think the helpers library becoming intimidating is best addressed via documentation, examples, and guides. In particular, it may be nice to have automated docs similar to the Nixpkgs Manual, Noogle, etc.

That said, if there are specific functions that are so simple and/or "single use" that it makes no sense to have a prime variant, we can drop them on a case-by case basis while reviewing this PR (or even revert them after it's merged).

@GaetanLepage
Copy link
Member

If we avoid adding functions for this reason alone, we will end up making our lives more difficult in the long-run.

Sure.

I think the helpers library becoming intimidating is best addressed via documentation, examples, and guides. In particular, it may be nice to have automated docs similar to the Nixpkgs Manual, Noogle, etc.

Definitely true.

That said, if there are specific functions that are so simple and/or "single use" that it makes no sense to have a prime variant, we can drop them on a case-by case basis while reviewing this PR (or even revert them after it's merged).

I agree. Also, even though we export the helpers publicly, I don't think we should be too worried of breaking users' workflow when changing them. mkRaw is probably used by some people, but apart from this, I doubt many are used outside the project. Especially the option declaration stuff.

@traxys
Copy link
Member

traxys commented Jun 1, 2024

Yeah I don't really have a problem with a growing number of helpers, as long as we know which one to apply

@MattSturgeon MattSturgeon merged commit 297aa6d into nix-community:main Jun 1, 2024
@MattSturgeon MattSturgeon deleted the default_nulls branch June 1, 2024 13:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants