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

Set environment variables for scripts #802

Merged
merged 5 commits into from
Jan 30, 2021

Conversation

fwsmit
Copy link
Member

@fwsmit fwsmit commented Jan 7, 2021

Addresses #767 and #653. This PR sets environment variables for scripts to use. Let me know if I have to add more or if the naming should be changed. I still need to do the documentation, but I wanted to get some comments before that.

@codecov-io
Copy link

codecov-io commented Jan 7, 2021

Codecov Report

Merging #802 (bc3de38) into master (e90f605) will increase coverage by 0.16%.
The diff coverage is 70.52%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #802      +/-   ##
==========================================
+ Coverage   59.48%   59.64%   +0.16%     
==========================================
  Files          36       36              
  Lines        5800     5880      +80     
==========================================
+ Hits         3450     3507      +57     
- Misses       2350     2373      +23     
Flag Coverage Δ
unittests 59.64% <70.52%> (+0.16%) ⬆️

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

Impacted Files Coverage Δ
src/notification.c 60.05% <0.00%> (-2.83%) ⬇️
src/utils.c 86.50% <0.00%> (-5.09%) ⬇️
src/icon.c 70.12% <100.00%> (+3.01%) ⬆️
test/icon.c 95.26% <100.00%> (+1.66%) ⬆️

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 e90f605...bc3de38. Read the comment docs.

@fwsmit fwsmit force-pushed the environment-variables branch 2 times, most recently from 6e5ebfd to 38ed1d3 Compare January 21, 2021 15:51
@fwsmit
Copy link
Member Author

fwsmit commented Jan 21, 2021

Turns out, setenv can block if you feed it NULL. So I made a safe setenv function to prevent that. Also added some documentation.

@fwsmit fwsmit force-pushed the environment-variables branch 4 times, most recently from dddb51e to ac6f1fd Compare January 26, 2021 20:20
@fwsmit
Copy link
Member Author

fwsmit commented Jan 26, 2021

I added the icon path function to this pr as well. Tests fail, because the old behaviour of the pixbuf creation was to continue searching the icon path when an invalid icon was found. This behaviour changed to returning the path to the invalid icon, thus not being able to create the pixbuf.
If you want I could add a check if the icon is valid to the path search, but I don't think it's very important.

@fwsmit fwsmit force-pushed the environment-variables branch from ac6f1fd to beaba34 Compare January 26, 2021 20:50
@tsipinakis
Copy link
Member

CI appears to be failing now.

@fwsmit
Copy link
Member Author

fwsmit commented Jan 27, 2021

CI appears to be failing now.

See #802 (comment)

@tsipinakis
Copy link
Member

See #802 (comment)

Whoops, that should teach me to review code at 1 am 🙈

That's fine, I don't expect that there will be many invalid icons and, but please fix the tests :)

@fwsmit fwsmit force-pushed the environment-variables branch 2 times, most recently from 24b3e8d to 0486ca5 Compare January 29, 2021 09:38
@fwsmit fwsmit force-pushed the environment-variables branch from 0486ca5 to bc3de38 Compare January 29, 2021 09:43
@fwsmit
Copy link
Member Author

fwsmit commented Jan 29, 2021

I forgive you :). The tests should now be fixed

Copy link
Member

@tsipinakis tsipinakis left a comment

Choose a reason for hiding this comment

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

LGTM

@tsipinakis tsipinakis merged commit 814e620 into dunst-project:master Jan 30, 2021
@fwsmit fwsmit mentioned this pull request Jun 7, 2021
@fwsmit fwsmit deleted the environment-variables branch February 16, 2022 09:30
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.

3 participants