Add command line color option #1

Merged
sahin/binary-clock-wallpaper
loicreynier merged 3 commits into main from main 2022-04-08 12:16:23 +03:00
Conversation 5 Commits 3 Files changed 4 +26 -6
loicreynier commented 2022-04-07 14:55:33 +03:00 (Migrated from github.com)

This add a command line option to specify the accent color:

python3 binary_clock_wallpaper.py --color 35d8dd

The color option can also be passed too the shell script:

./set_binary_wallpaper.sh --color

Command line arguments are captured using sys.argv, I figured using a library as argparse was a little bit overkill to add just one option but I could replace this by a real argument parser (with better error capturing and --help message) if you are interested.

Thank you for this cool little script!

This add a command line option to specify the accent color: ```shell python3 binary_clock_wallpaper.py --color 35d8dd ``` The color option can also be passed too the shell script: ```shell ./set_binary_wallpaper.sh --color ``` Command line arguments are captured using `sys.argv`, I figured using a library as `argparse` was a little bit overkill to add just one option but I could replace this by a real argument parser (with better error capturing and `--help` message) if you are interested. Thank you for this cool little script!
sahinakkaya commented 2022-04-07 15:27:55 +03:00 (Migrated from github.com)

Thank you for taking time to contribute! I will review this later this evening. Also thanks for your nice words.

Thank you for taking time to contribute! I will review this later this evening. Also thanks for your nice words.
sahinakkaya (Migrated from github.com) reviewed 2022-04-07 22:49:36 +03:00
binary_clock_wallpaper.py
@@ -7,12 +9,12 @@ base_image_path = os.path.join(base_dir, "img", "base.png")
result_image_path = os.path.join(base_dir, "img", "result.png")
sahinakkaya (Migrated from github.com) commented 2022-04-07 22:49:36 +03:00

I think this should not print "Invalid command line arguments". One can run script without any argument and it should be fine. Maybe you can change it to something like "No color specified, using default color value"

I think this should not print "Invalid command line arguments". One can run script without any argument and it should be fine. Maybe you can change it to something like "No color specified, using default color value"
sahinakkaya (Migrated from github.com) reviewed 2022-04-08 01:05:28 +03:00
sahinakkaya (Migrated from github.com) left a comment

Thank you for your contribution! I will merge this if you can address the issues I mentioned. If you don't agree and have a good reason about it, I am more than happy to hear about it.

Thank you for your contribution! I will merge this if you can address the issues I mentioned. If you don't agree and have a good reason about it, I am more than happy to hear about it.
binary_clock_wallpaper.py
@@ -7,12 +9,12 @@ base_image_path = os.path.join(base_dir, "img", "base.png")
result_image_path = os.path.join(base_dir, "img", "result.png")
sahinakkaya (Migrated from github.com) commented 2022-04-08 00:58:52 +03:00

I am not sure whether validating the color should be our concern or not. Yeah, it is a nice feature but I think even if the color is not valid, PIL should handle it by throwing an error.

I am not sure whether validating the color should be our concern or not. Yeah, it is a nice feature but I think even if the color is not valid, PIL should handle it by throwing an error.
sahinakkaya (Migrated from github.com) commented 2022-04-08 01:00:42 +03:00

Another good reason to remove this regex is being able to use generic color names such as "red", "green" etc. PIL knows how to process them but this regex does not allow them.

Another good reason to remove this regex is being able to use generic color names such as "red", "green" etc. PIL knows how to process them but this regex does not allow them.
loicreynier commented 2022-04-08 11:56:43 +03:00 (Migrated from github.com)

Following your comments, I remove the error if no arguments are passed (args = []) and the color validation. If the color is specified with a hex code, the # must now be specified:

python3 binary_clock_wallpaper.py --color "#35d8dd"
Following your comments, I remove the error if no arguments are passed (`args = []`) and the color validation. If the color is specified with a hex code, the `#` must now be specified: ```shell python3 binary_clock_wallpaper.py --color "#35d8dd" ```
sahinakkaya (Migrated from github.com) approved these changes 2022-04-08 12:12:55 +03:00
sahinakkaya (Migrated from github.com) left a comment

Thank you!

Thank you!
Sign in to join this conversation.
Reviewers
No reviewers
sahinakkaya
Assignees
Clear assignees
No assignees
Labels
Clear labels bug Something isn't working documentation Improvements or additions to documentation duplicate This issue or pull request already exists enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed invalid This doesn't seem right question Further information is requested wontfix This will not be worked on
No labels bug documentation duplicate enhancement good first issue help wanted invalid question wontfix
Projects
Clear projects
No items
No project
Milestone
Clear milestone
No items
No milestone
Dependencies

No dependencies set

Notifications
1 participant
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Reference
sahin/binary-clock-wallpaper!1
No description provided.