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

Use a module system for the different integrations #205

Merged
merged 13 commits into from
Jun 20, 2024
Merged

Use a module system for the different integrations #205

merged 13 commits into from
Jun 20, 2024

Conversation

thufschmitt
Copy link
Member

Uses a module-system (inspired by the NixOS one) to make the whole thing a cleaner:

  1. Clearly separates the options provided by the modules from their implementations
  2. Actually use merging for combining modules rather than abusing contract application (avoids issues like List equality in contracts can cause some big slowdowns when applied several times tweag/nickel#1930)
  3. Gives a standard way of writing modules so that they can be unified rather than each having a different ad-hoc structure.

@thufschmitt thufschmitt marked this pull request as ready for review June 17, 2024 08:26
@thufschmitt thufschmitt mentioned this pull request Jun 17, 2024
Merged
@thufschmitt thufschmitt force-pushed the modules branch 2 times, most recently from a026bc0 to cc9c94c Compare June 17, 2024 12:06
@thufschmitt thufschmitt mentioned this pull request Jun 17, 2024
Merged
@thufschmitt thufschmitt mentioned this pull request Jun 19, 2024
Merged
yannham
yannham approved these changes Jun 19, 2024
View reviewed changes
Copy link
Contributor

@yannham yannham left a comment

Choose a reason for hiding this comment

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

LGTM! The migration seems rather straightforward.

lib/modules/main.ncl Show resolved Hide resolved
lib/modules/main.ncl Show resolved Hide resolved
lib/modules/main.ncl Outdated Show resolved Hide resolved
lib/modules/main.ncl Show resolved Hide resolved
lib/modules/main.ncl Outdated Show resolved Hide resolved
thufschmitt and others added 2 commits June 19, 2024 18:03
@thufschmitt @yannham
Apply suggestions from code review
9a4ee05 
Co-authored-by: Yann Hamdaoui <[email protected]>
Remove a useless module merge function
e43b559 
Just an alias for `&`, but which breaks the LSP
thufschmitt and others added 2 commits June 19, 2024 18:38
@thufschmitt
Only export the flake field of the nickel config (#208)
04bfa74 
Makes it clear what Nix has (or not) access to.

Also makes it possible in the future to fearlessly use enums in the
config, as they won't be exported
@thufschmitt @yannham
Add an “Example” header to a documentation field
88b3dc7 
Align with the usual Nickel convention

Co-authored-by: Yann Hamdaoui <[email protected]>
@thufschmitt thufschmitt added this pull request to the merge queue Jun 20, 2024
Merged via the queue into main with commit b0585da Jun 20, 2024
1 check passed
@thufschmitt thufschmitt deleted the modules branch June 20, 2024 16:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yannham yannham yannham approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@thufschmitt @yannham