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

stylix: clean up fromOs #407

Merged
merged 7 commits into from
Jun 11, 2024
Merged

stylix: clean up fromOs #407

merged 7 commits into from
Jun 11, 2024

Conversation

danth
Copy link
Owner

@danth danth commented May 31, 2024

A common question/issue is that the fallback default value for stylix.image is null, which is not a path or package. This produces a type error when it should simply state that the option has not been set.

This PR removes the fromOs function, and replaces it with a module which copies listed settings.

  • The module is only loaded on the OS side, so it doesn't need to check that an OS configuration exists to copy from.
  • The module injects extra Home Manager modules alongside the main one, so nothing is evaluated when using Home Manager standalone.
  • All the options to be copied are listed in a central location.
  • Two of the options are copied conditionally, to match previous behaviour.
  • Copied values use mkDefault, again to match previous behaviour.

Fixes #406
Fixes #345

@danth danth marked this pull request as ready for review May 31, 2024 19:08
stylix/home-manager-integration.nix Outdated Show resolved Hide resolved
stylix/home-manager-integration.nix Outdated Show resolved Hide resolved
@danth
Copy link
Owner Author

danth commented May 31, 2024

Regarding my two comments above, the easiest solution would be to merge followSystem and autoImport into a single option, such as enableHomeManagerIntegration.

Copy link
Collaborator

@trueNAHO trueNAHO left a comment

Choose a reason for hiding this comment

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

LGTM

@danth

This comment was marked as resolved.

@danth

This comment was marked as resolved.

@danth danth mentioned this pull request Jun 10, 2024
6 tasks
@danth
Copy link
Owner Author

danth commented Jun 10, 2024

This now respects followSystem as before, however it only works when autoImport is also being used.

I doubt anyone was actually using followSystem without autoImport, given that it was only applicable to multi-user configurations, and even then was probably not the best way to organise things.

I think this is ready to merge :)

@danth danth merged commit f060e40 into master Jun 11, 2024
9 checks passed
@danth danth deleted the clean-up-fromos branch June 11, 2024 01:21
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.

Image needs to be set on rebuild Error with stylix.image: not of type 'path or package convertible to it
2 participants