-
Notifications
You must be signed in to change notification settings - Fork 342
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
Resolve paths for default icon #1210
Resolve paths for default icon #1210
Conversation
It seems a little weird to add custom code for expanding the Adding a |
I was on the edge whether I should expand
|
I took a closer look at |
904ce6f
to
4a1ee5d
Compare
I've implemented a version of the resolution using
|
4a1ee5d
to
dc64c31
Compare
Codecov Report
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. @@ Coverage Diff @@
## master #1210 +/- ##
=======================================
Coverage 66.03% 66.03%
=======================================
Files 46 46
Lines 7595 7595
=======================================
Hits 5015 5015
Misses 2580 2580
Flags with carried forward coverage won't be shown. Click here to find out more. 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
I think it's better to keep the |
Currently is always treated as a plain string so no expansion of `~` happens. The real resolution (name of icon vs. path to icon) is done later when the notification is about to be rendered.
dc64c31
to
ca09495
Compare
Thanks, this makes it easy to review :) |
This (partially) fixes #1173 - support for expanding variables is done in #1215
default_icon
was erroneously declared as being a plain string while it can also be a full path. Thus changing its type should solve the described issue already (without any side effects I could currently see).