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

Add recursive config includes #995

Merged
merged 4 commits into from
Jun 23, 2021
Merged

Add recursive config includes #995

merged 4 commits into from
Jun 23, 2021

Conversation

OskarCarl
Copy link
Contributor

Hey,
so I've tried my hand at implementing recursive configuration imports with an "include" array:

"include": [
	"$HOME/.config/waybar/included_file"
]

Disclaimer: I have next to no experience in C++ so I very possibly could've built an error into this. I would welcome it very much if you would be able to have a close look at it.

Some things:

  • It does not break existing configs, at least not in my tests.
  • It simply tries to recursively merge configuration files specified in an "include" array with the main file.
  • Duplicate keys are overwritten. Since it's a bottom-up recursion, the main file overrides included files.
  • It is a tiny bit slower for standard configurations since it recursively copies instead of directly assigning the config.
  • Cyclic imports completely break it.

If you have comments or would like changes I'd be happy to work on it some more. In that case I would highly appreciate if you could point me into the right direction since I'll probably need to learn about C++ behaviour.
Thanks!

@OskarCarl
Copy link
Contributor Author

Hey, just wanted to ask whether there is interest in this PR. It would substantially simplify maintenance of my dotfiles.

  • Is there any obvious issue that I'm missing? Anything I can do?
  • Is there general interest in this functionality or is there something prohibiting it that I'm not aware of?
  • Is it just down to a lack of time?

I've added a workaround so it's not completely broken by circular imports. It simply errors out and prints a message at a depth of 101 imports.

Thanks!
(Pinging @Alexays jic it was an oversight. Sorry if I'm intruding)

@Alexays
Copy link
Owner

Alexays commented Jun 23, 2021

Sorry for the delay, it's a nice addition!
Can you also update the GitHub wiki?
Thanks!

@Alexays Alexays merged commit 36857ae into Alexays:master Jun 23, 2021
@OskarCarl
Copy link
Contributor Author

Great, thanks! Will do.

@rieje
Copy link

rieje commented Jul 6, 2021

If waybar can support e.g. $hostname or $uname -n this would be perfect for version control. You can have a config contain common settings and a

"include": [
	    "$HOME/.config/waybar/config-$hostname"
]

For example, in sway one can have in their main config file: include "$HOME/.config/sway/config-$(hostname)"

To be honest, polybar's method is vastly preferred: a config that contains everything (multiple bars configured) and then you simply pass an argument specifying which bar you want. It also supports environment variables so you can e.g. MONITOR=HDMI-A-0 polybar <hostname> to override the default MONITOR in the config.

@rieje
Copy link

rieje commented Jul 8, 2021

I'm sure you already considered this, but it would be more intuitive and useful to have duplicate options prioritize the deepest included file over the main config file. This is how it works for most if not all programs that involve hierarchical/recursive overriding of options. The main config file serves as a general template to be overridden by more specialized configs.

It is a little bizarre to need to omit options in the main config file for such a purpose, resulting in the user needing to define options for all include files involved (assuming each include file contains host-specific settings). To me, the purpose of an include file is to reduce the amount of duplicate option definitions and make the distinction clear how each include file is different from the general settings but with options in the main config overriding those in the include files, it doesn't seem to achieve either.

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.

None yet

3 participants