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

Allow for reading theme information from standalone files (cf PR #366) #611

Merged
merged 6 commits into from
Apr 28, 2020
Merged

Conversation

PicoJr
Copy link
Contributor

@PicoJr PicoJr commented Apr 25, 2020

Allow for reading theme information from standalone files (cf PR #366)

This PR contains the original 2 commits from atheriel (rebased).

Thanks to @atheriel who did the heavy-lifting =)

It should be compatible with previous config file format: see unit tests in src/config.rs.

I took the opportunity to move has_command from pacman.rs to utils.rs.

I also added some unit tests for color_from_rgba, I changed the code a little to fix a panic when input is too short: color_from_rgba("rust") would panic...

atheriel and others added 5 commits April 25, 2020 12:01
This paves the way for modifying what keys are permitted in the TOML
entry for [theme].

(cherry picked from commit 109710d)
This introduces a new `theme.file` key to the TOML configuration. We
look in the following locations for this file:

- The actual file location, if it is a real path.
- $XDG_CONFIG_HOME/i3status-rs/themes; and
- /usr/share/i3status-rs/themes

The theme file must be "complete", e.g. contain all of the keys, for it
to be serialized correctly.

(cherry picked from commit f36e2e3)
run cargo fmt

(cherry picked from commit d1dad49)
src/themes.rs Outdated Show resolved Hide resolved
src/util.rs Outdated Show resolved Hide resolved
@atheriel
Copy link
Collaborator

This looks good to me in general, thanks for taking up the cause. Two concerns:

  1. As decided in Provide a default config file paths #465, we should use i3status-rust for config paths, not i3status-rs.
  2. This introduces about a dozen new dependencies to run the unit tests -- is that necessary?

@PicoJr
Copy link
Contributor Author

PicoJr commented Apr 26, 2020

This looks good to me in general, thanks for taking up the cause. Two concerns:

1. As decided in #465, we should use `i3status-rust` for config paths, not `i3status-rs`.

Config path fixed in latest commit.

2. This introduces about a dozen new dependencies to run the unit tests -- is that necessary?

I wouldn't say it is necessary: It might be possible to adapt the tests so that no temporary files are required (we wouldn't depend on assert_fs)

I would argue it's only dev-dependencies: assert_fs is not required to build the project, only to run the tests.
Nevertheless it still negatively impacts CI validation time (by how much I don't know).

If this new dependency is an issue don't hesitate to reach me out :)

@ammgws
Copy link
Collaborator

ammgws commented Apr 26, 2020

I would argue it's only dev-dependencies: assert_fs is not required to build the project, only to run the tests.

I think it's fine if it's just a dev dep. Also it could lead the way for tests to be introduced in the other blocks too. @atheriel ?

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