Skip to content

Conversation

TheDoctor314
Copy link
Contributor

@TheDoctor314 TheDoctor314 commented Sep 28, 2021

What type of PR is this? (check all applicable)

  • Refactor
  • Feature
  • Bug Fix
  • Optimization
  • Documentation Update
  • Other: Replace this with a description of the type of this PR

Description

This PR wil add the env-* config option in the script module

Related Issues & Documents

Fixes #2090.

Documentation (check all applicable)

  • This PR requires changes to the Wiki documentation (describe the changes)
  • This PR requires changes to the documentation inside the git repo (please add them to the PR).
  • Does not require documentation changes

Document the env-* option for the 'script' module in the wiki.

Returns a list of key-value pairs starting with a prefix.
If you have in config:
```
env-FOO = bar
env-CAT = dog
```

then `get_with_prefix("env-")` will return
`[{"FOO", "bar"}, {"CAT", "dog"}]`
@TheDoctor314
Copy link
Contributor Author

@patrick96 I have a question. How do I test a new config option?

@patrick96
Copy link
Member

Do you mean adding unit tests for it? There isn't really any infrastructure for that unfortunately. I think it's fine doing this without tests for now.

At some point we should write some code that allows us to easily construct config instances with certain given values.

@codecov
Copy link

codecov bot commented Sep 28, 2021

Codecov Report

Merging #2512 (4554617) into master (55eb19f) will increase coverage by 0.12%.
The diff coverage is 4.76%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2512      +/-   ##
==========================================
+ Coverage   10.10%   10.22%   +0.12%     
==========================================
  Files         147      145       -2     
  Lines       10235    10110     -125     
==========================================
  Hits         1034     1034              
+ Misses       9201     9076     -125     
Flag Coverage Δ
unittests 10.22% <4.76%> (+0.12%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
include/components/config.hpp 0.00% <0.00%> (ø)
include/modules/script.hpp 0.00% <ø> (ø)
src/modules/script.cpp 0.00% <0.00%> (ø)
src/utils/process.cpp 18.18% <0.00%> (-0.49%) ⬇️
src/utils/command.cpp 53.19% <50.00%> (ø)
include/components/types.hpp 2.43% <0.00%> (-0.60%) ⬇️
src/main.cpp 0.00% <0.00%> (ø)
src/utils/env.cpp 100.00% <0.00%> (ø)
src/components/bar.cpp 0.00% <0.00%> (ø)
src/components/renderer.cpp 0.00% <0.00%> (ø)
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 55eb19f...4554617. Read the comment docs.

Before passing the cmd to exec() we set the required environment
variables.

Also add the test for it.
This stores the key-value pairs specified for the script module.
The command to be executed must pass on this argument.
@TheDoctor314 TheDoctor314 marked this pull request as ready for review September 28, 2021 19:21
Copy link
Member

@patrick96 patrick96 left a comment

Choose a reason for hiding this comment

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

Very nice. With tests even 🎆

I went ahead and modified the config loading a bit by templating the value type. Otherwise everything else looks good.

Thanks a lot 😃

@patrick96 patrick96 merged commit 626c55f into polybar:master Sep 28, 2021
@TheDoctor314 TheDoctor314 deleted the env-in-script-mod branch September 29, 2021 10:44
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.

[Feature request] Environment field in custom scripts
2 participants