Skip to content

feat(wrapperModules.oh-my-posh): init#500

Open
zenoli wants to merge 1 commit intoBirdeeHub:mainfrom
zenoli:omp-wrapper-module
Open

feat(wrapperModules.oh-my-posh): init#500
zenoli wants to merge 1 commit intoBirdeeHub:mainfrom
zenoli:omp-wrapper-module

Conversation

@zenoli
Copy link
Copy Markdown
Contributor

@zenoli zenoli commented May 3, 2026

So this is basically a copy of the starship wrapper except:

  • "presets" (starship) are named "themes" in omp
  • Starship uses TOML, omp uses json by default, which made the build script a bit simpler.

One thing that is missing though:

Omp also allow passing yaml files to --config. I've seen no importYaml equivalent to importJSON/importTOML.
So in order to support YAML we might also allow another option configFile which can be either JSON/TOML/YAML (kinda like I had in my first proposal for the starship module, where @BirdeeHub argued - rightly so - that it was not needed since users can simply use importTOML if they want to write a native toml file.

I was thinking reading from this new option, and normalize the yaml/toml/json to json using yq in the builder and also generalize the order field to define the order between [theme nix-settings configFile].

Thoughts, @BirdeeHub ?

Comment thread wrapperModules/o/oh-my-posh/module.nix Outdated
@BirdeeHub
Copy link
Copy Markdown
Owner

BirdeeHub commented May 4, 2026

given the lack of importYAML, having a lib.types.either lib.types.package lib.types.path is fine. It doesn't support an impure path though, because we have to merge it at build time, hence not using wlib.types.stringable

What would the criteria be to determine which type the provided file is of those 3? File extension? Option? Either?

Somewhat busy tonight, will be reviewing stuff again tomorrow night probably

@zenoli
Copy link
Copy Markdown
Contributor Author

zenoli commented May 4, 2026

What would the criteria be to determine which type the provided file is of those 3? File extension? Option? Either?

I was thinking file extension.

config.configFile = ./my-config.yaml;

I try to implement an extra option on top, but only if it is opt-in.

I don't want them to write

config.configFile = {
    path = ./my-config.yaml;
    fileType = "yaml";
}

They should use the single-line syntax from above where fileType will be null and determined by extension.
And they can option in to the longer second syntax if they choose to. I probably can do that by using a spec and path as the main field?

@BirdeeHub
Copy link
Copy Markdown
Owner

I try to implement an extra option on top

Dont worry about it, file extension is fine.

@zenoli zenoli force-pushed the omp-wrapper-module branch 2 times, most recently from e38691e to 47c78bf Compare May 4, 2026 17:21
@zenoli zenoli marked this pull request as ready for review May 4, 2026 17:22
@zenoli zenoli force-pushed the omp-wrapper-module branch from 47c78bf to 0739835 Compare May 4, 2026 17:24
@zenoli
Copy link
Copy Markdown
Contributor Author

zenoli commented May 4, 2026

Dont worry about it, file extension is fine.

Done!

Comment thread ci/test-lib.nix Outdated
Comment thread wrapperModules/o/oh-my-posh/module.nix Outdated
@zenoli zenoli force-pushed the omp-wrapper-module branch 2 times, most recently from 10e0ac3 to b7fe76e Compare May 9, 2026 17:49
@zenoli
Copy link
Copy Markdown
Contributor Author

zenoli commented May 9, 2026

Ok, config merging is fully reworked.

In a nutshell, if you specify multiple settings they will be chained together using the .extends key in the order specified in config.order.

In case you specified an .extends key explictly, we don't modify it (the chain is "broken").

See the description and tests (there are plenty :-)) for more details.

The main use case it, that people might want to use a theme but override some stuff (which was not really possible in my initial version). Since most of the omp settings are in nested lists (segments within blocks) it is a bit clumsy to override them.
I first tried to recreate the mergin logic described in the link above but that was too much work. So making use of the native "extends" feature felt more natural.

@zenoli zenoli force-pushed the omp-wrapper-module branch 3 times, most recently from 8eb41e0 to 7a3e3b9 Compare May 10, 2026 16:07
add check.nix

implement omp + checks

fix fileContains

add configFile option

clean up tests

simplify

use optionalString

use omp-native "extends" feature to merge configs

add config-chain folder

preserve original names in config chain

add foo.omp.json

handle reverse traversal of ordered settings

remove config-chain dir if empty

ensure that settings get copied to config-chain

properly copy settings.json

remove nix store hash from config file

update tests

cleanup test

refactor

simplify tmp file

rename _omp_out --> dst

simplify copy case

rework json normalization

fix tests

wip

cleanup

nix recursion rewrite

recurse from the end of the list

simplify chainFiles

cleanup unreachable configs

improve docs

linting

improve docs

cleanup

improve errMsgs in testlib

add config.json test

add test for breaking config-chain

add test for extending segments

remove dummie configs
@zenoli zenoli force-pushed the omp-wrapper-module branch from 7a3e3b9 to cc79e71 Compare May 10, 2026 16:20
@zenoli
Copy link
Copy Markdown
Contributor Author

zenoli commented May 10, 2026

Did some cleanup on the tests. I think this is now ready for review.

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.

2 participants