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.

Sign up for GitHub

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

PB-1083: Removed default icon set 'babs' from UNLISTED_ICON_SET #93

Merged
merged 4 commits into from
Nov 15, 2024
Merged

PB-1083: Removed default icon set 'babs' from UNLISTED_ICON_SET #93

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b878f12
PB-1083: Removed default icon set 'babs' from UNLISTED_ICON_SET
Nov 13, 2024
3686f51
PB-1083 : re-factored UNLISTED_ICON_SETS to be fetched with a function
Nov 14, 2024
cf84773
PB-1083: ran a 'make format' to fix an issue highlighted by CodeBuild
Nov 15, 2024
c7caeaf
PB-1083: Re-factored fetch_and_clean_unlisted_sets to be generic
Nov 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -192,4 +192,4 @@ The service is configured by Environment Variable:
| WSGI_TIMEOUT | `5` | WSGI timeout. |
| GUNICORN_TMPFS_DIR | `None` |The working directory for the gunicorn workers. |
| WSGI_WORKERS | `2` | The number of workers per CPU. |
| UNLISTED_ICON_SETS | `'babs'`| Comma separated list of icon set to un-list. Those sets won't be listed in the /sets endpoint. |
| UNLISTED_ICON_SETS | | Comma separated list of icon set to un-list. Those sets won't be listed in the /sets endpoint. |
5 changes: 4 additions & 1 deletion app/settings.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,10 @@
)

COLORABLE_ICON_SETS = ['default']
UNLISTED_ICON_SETS = os.environ.get('UNLISTED_ICON_SETS', 'babs').split(',')

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]
Copy link
Contributor

Choose a reason for hiding this comment

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

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 == '' ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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 ''

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 ''

At this point it's better to have a separate function I would say, that's too much for one line imo


ICON_SET_LANGUAGE = {'babs-v2-de': 'de', 'babs-v2-fr': 'fr', 'babs-v2-it': 'it'}
DEFAULT_COLOR = {"r": '255', "g": '0', "b": '0'}
DEFAULT_ICON_SIZE = 48
Expand Down