PB-1083: Removed default icon set 'babs' from UNLISTED_ICON_SET #93
Conversation
This change helps us avoid changing UNLISTED_ICON_SET's content on both the service and the k8s configuration. In order to achieve this, I'm assigning an empty string '' (since os.environ.get requires a default value), and removing that string with a simple loop.
app/settings.py
Outdated
| UNLISTED_ICON_SETS = os.environ.get('UNLISTED_ICON_SETS', '').split(',') | ||
| UNLISTED_ICON_SETS = [icon_set for icon_set in UNLISTED_ICON_SETS if icon_set] |
There was a problem hiding this comment.
can we bunch these two lines into one?
also if icon_set might not be enough to filter out a blank value given by the os.environ.get as the default there is ''. Maybe something like if not icon_set == '' ?
There was a problem hiding this comment.
We could bunch these lines and have something like :
UNLISTED_ICON_SETS = [icon_set for icon_set in os.environ.get('UNLISTED_ICON_SETS', '').split(',') if icon_set]
But I find that very hard to read, and even my original implementation is far from perfect. I would like to write a function fetchAndCleanUnlistedSets that returns a clean UNLISTED_ICON_SETS, therefore making settings.py more readable. I'm just not sure which file I should write it (if I should write the function that is)
From my testing, if icon_set correctly filters out the blank and UNLISTED_ICON_SETS comes out as an empty array ("UNLISTED_ICON_SETS = []"). Empty strings are considered "false" in python so I don't see why it wouldn't work.
if not icon_set == '' would also work, but is more verbose and less readable in my mind (I usually favor positive conditions as we are quicker to grasp them). For what reason should we favor it over the other?
There was a problem hiding this comment.
a possibility to make a one line would be the following :
UNLISTED_ICON_SETS = [icon_set for icon_set in os.environ.get('UNLISTED_ICON_SETS', '').split(',') if os.environ.get('UNLISTED_ICON_SETS')]
the default for os.environ.get('') is none, which means we would have an empty array if the environment variable is not set up. With the current implementation, you'd get an array containing ''
There was a problem hiding this comment.
a possibility to make a one line would be the following :
UNLISTED_ICON_SETS = [icon_set for icon_set in os.environ.get('UNLISTED_ICON_SETS', '').split(',') if os.environ.get('UNLISTED_ICON_SETS')]the default for
os.environ.get('')isnone, which means we would have an empty array if the environment variable is not set up. With the current implementation, you'd get an array containing''
At this point it's better to have a separate function I would say, that's too much for one line imo
This change improves readability of settings.py, and removes the double assignement, with a cleaner overall routine. UNLISTED_ICON_SETS is now assigned in settings.py using the function fetch_and_clean_unlisted_sets.
app/helpers/icons.py
Outdated
|
|
||
| def fetch_and_clean_unlisted_sets(): | ||
| unlisted_icon_sets = os.environ.get('UNLISTED_ICON_SETS', '').split(',') | ||
| cleaned_unlisted_icon_sets = [icon_set for icon_set in unlisted_icon_sets if icon_set] |
There was a problem hiding this comment.
Have you tested how this behaves when it receives nothing through the UNLISTED_ICON_SETS env variable?
I'm still not quite comfortable with this last if icon_set and the default value being an empty string...
There was a problem hiding this comment.
From my testing it behaves as intended, here's a demonstration :
(with two unlisted sets)
(with none)
We can also see them absent from sets when the route is pinged.
If I remove the default value, I'm unable to call .split() as the object could be a None (if the key isn't found). This could also trigger KeyError exceptions (https://stackoverflow.com/a/4907053). We could handle that exception with something like :
try:
os.environ["FOO"]
except KeyError:
print "Please set the environment variable FOO"
sys.exit(1)
But that would generate added complexity and error management that may not be necessary, if we simply assign a default value and purge it, which seems to be the common way to handle this.
Assigning a different default type doesn't work either as we can't call split() on a list, so we would have to cast the list to a string before splitting which is less efficient than the current implementation.
In regards to if icon_set, I'm following this clean coding practice, which from my (limited) experience makes code easier to read. It is also deemed "the more pythonic way" of checking for empty strings in a list (see https://stackoverflow.com/a/3845449)
app/helpers/icons.py
Outdated
| @@ -14,3 +17,9 @@ def get_icon_template_url(base_url='', with_color=True): | |||
| color_part = "-{r},{g},{b}" | |||
| return f"{get_icon_set_template_url(base_url)}/icons/{{icon_name}}" \ | |||
| f"@{{icon_scale}}{color_part}.png" | |||
|
|
|||
|
|
|||
| def fetch_and_clean_unlisted_sets(): | |||
There was a problem hiding this comment.
I would generalize this function. Something like split_string, or maybe you can find something better. The function would then receive a string argument that is being split, and then call it with os.environ.get('UNLISTED_ICON_SETS', '').
Previously, fetch_and_clean_unlisted_sets was specifically only usable to fetch an environment variable and clean it. In order to address this, this workflow has been separated. In settings.py, the environment variable is fetched and passed onto split_and_clean_string.
sami-nouidri-swisstopo
deleted the
feat-PB-1083-correct-default-unlisted-value
branch
This change helps us avoid changing UNLISTED_ICON_SET's content on both the service and the k8s configuration.
In order to achieve this, I'm assigning an empty string '' (since os.environ.get requires a default value), and removing that string with a simple loop.
[X] Local tests pass
[X] Manual testing done on both docker and local server