Implement hex color conversion #1270

idonec · 2023-08-12T12:14:54Z

Added utility functions for conversion from hex color value to API-accepted color dicts and converting 0-255 RGB colors to hex color values.

Added the set_tab_color worksheet function to update tabs colors using hex values.

Included relevant documentation, tests, doctests, and cassette.

As mentioned in #1199 and #1213
I'm interested in the maintainers' perspective on non-breaking ways of incorporating this change
into other related sheet formatting functionality.

Thank you for your work on this great library.

Added utility functions for conversion from hex color value to API-accepted color dicts and converting 0-255 RGB colors to hex color values. Added the `set_tab_color` worksheet function to update tabs colors using hex values. Included relevant documentation, tests, doctests, and cassette

alifeee

thank you for this PR! :) your code is nice and your tests nicer

I have submitted a few comments, and also have these remarks:

A question we must ask is if we want to make this a breaking change (force everyone to use hex from v6.0.0) or an addition (new methods/updated arguments for using hex)

In this context, we can either

case 1 - breaking change
1. change Worksheet.update_tab_color to use only hex colors (we must provide a deprecation warning and guidance for which function to use to convert dict -> hex to ease migration).
2. change Worksheet.tab_color to return hex instead of dict
case 2 - addition
1. change update_tab_color to also accept hex colors (@lavigne958 I do not know how complex you wish the typing to be), and we can write a code fork for isinstance(dict) and isinstance(hex) (convert to dict) before sending the API
2. leave Worksheet.tab_color as is and inform the user (somewhere?) that there are functions in utils for converting this to a hex*

*one could also add an enum when creating the Worksheet object which specifies color type. This sounds too complex.

I prefer option 1 as I believe it is simpler in the long run. Using hex codes for colors is very standard, and it would be nice if that were the case everywhere, without users having to think about dictionaries. A major version release is also a good time to introduce such a breaking change like this.

We would love to know your thoughts, as a contributor! and also @lavigne958's thoughts.

thanks again for this great PR 💗

gspread/utils.py

alifeee · 2023-08-13T10:18:51Z

Additional note:

The "hex -> dict" function represents colors as 0.0-1.0. The "dict -> hex" represents them with 0-255. It seems to me like they should work the same to reduce confusion.

Now the hex convertion utility functions are symmetrical (dict <-> hex). Added support for 3-character hex values in `convert_hex_to_colors_dict`. Added typing and updated tests and docs accordingly.

idonec · 2023-08-14T23:04:40Z

I'm also leaning towards the first option. While it would introduce a breaking change, handling colors as hex values seems to be more intuitive and user-friendly.
Unless we account for the ability to do some sick color arithmetics I don't think this would result in significant losses.
Definitely would need a warning though.

gspread/utils.py

alifeee · 2023-08-15T15:31:19Z

tests/utils_test.py

@@ -295,3 +295,18 @@ def test_convert_hex_to_color(self):
        # raise ValueError on invalid hex characters
        with self.assertRaises(ValueError):
            utils.convert_hex_to_colors_dict("axbcde")
+
+    def test_fill_gaps(self):


this has snuck in from elsewhere. it is not relevant to this PR

alifeee · 2023-08-15T15:32:37Z

I'm also leaning towards the first option. While it would introduce a breaking change, handling colors as hex values seems to be more intuitive and user-friendly.

Agreed. For now, you can continue with this PR, and we will deal with any Deprecation warnings that need adding :)

lavigne958

thank you for this contribution this is very nice ! good work

I agree with the breaking change, this is now or in a far future.... no uses the dict decimal value like the API uses :-(

@alifeee do you agree we must first merge the PR that introduces the warning for breaking change then we merge this PR ? just in case 🤔

lavigne958 · 2023-08-15T23:00:02Z

tests/utils_test.py

+
+    def test_convert_hex_to_color(self):
+        hex = "#FF8000"
+        expected_color = {"red": 1, "green": 128 / 255, "blue": 0}


for the green value please use some random value with some decimals, just in case and not some rounded value if possible, something like: 0.49803922 which is the return value for the green field of one of my sheets.

alifeee · 2023-08-16T10:37:25Z

@alifeee do you agree we must first merge the PR that introduces the warning for breaking change then we merge this PR ? just in case 🤔

I think we are fine to merge this PR first. Deprecation warnings are tracked by the issue #1272.

Added rounding of the returned values in `convert_hex_to_colors` to 8 places after the decimal point. This matches the color values returned by the API. Updated values in `test_convert_hex_to_color`.

alifeee · 2023-08-16T14:50:44Z

personally I would stay away from rounding, as

as mentioned by #1199 (comment), Google inconsistently rounds the returned value. Thus, you would desire a test for each of the 256 possible values (perhaps with Pytest.Parametrize) to ensure that equality can be maintained.

However, this would be quite frivolous for a number the user never gets. However, it might be worth testing that equality is conserved in hex-space, ie, that setting "#343434" is not incorrectly rounded to "#353535", for example (I am not sure of the logic, whether it uses floor, ceil, or round).

alifeee · 2023-08-16T15:53:15Z

Having looked, it looks like rounding errors are fine as def convert_colors_to_hex_value rounds decimals to the closest fraction of 255.

I still think we should not use round in convert_hex_to_colors_dict for the reasons above.

Really, the tests should stay in hex only, as that is the only color format that we support (after this merge), so the only one we should test.

code by Ido Nechushtan <[email protected]> from #1270

lavigne958 · 2023-08-19T10:34:16Z

Having looked, it looks like rounding errors are fine as def convert_colors_to_hex_value rounds decimals to the closest fraction of 255.

I still think we should not use round in convert_hex_to_colors_dict for the reasons above.

Really, the tests should stay in hex only, as that is the only color format that we support (after this merge), so the only one we should test.

I understand that's what I thought too when I saw that. What do you want to test: only the conversation given a hex value I always get the same dict values ?
Or the other day around? 🤔

The test for `set_tab_color` has been updated to verify that the hex values of the colors returned from Google match the hex value given to the function. Removed rounding from (hex -> dict) utils function.

gspread/worksheet.py

Updated the `update_tab_color` function to use hex color values as parameter. Adjusted related test cases for `update_tab_color` and `clear_tab_color` to use the updated function. Removed redundant `set_tab_color` function along with its test.

alifeee

this is lovely now. thank you very much for your work :))

I am very happy with the code here. I will let @lavigne958 merge this.

alifeee self-requested a review August 13, 2023 08:59

alifeee requested changes Aug 13, 2023

View reviewed changes

gspread/utils.py Outdated Show resolved Hide resolved

gspread/utils.py Show resolved Hide resolved

gspread/utils.py Outdated Show resolved Hide resolved

gspread/utils.py Outdated Show resolved Hide resolved

gspread/utils.py Outdated Show resolved Hide resolved

Merge branch 'feature/release_6_0_0' into feature/release_6_0_0

fa90df2

update cassette (after merge)

bcbf639

alifeee assigned idonec Aug 14, 2023

Modify convert_colors_to_hex_value to accept floats

a3e12e2

Now the hex convertion utility functions are symmetrical (dict <-> hex). Added support for 3-character hex values in `convert_hex_to_colors_dict`. Added typing and updated tests and docs accordingly.

alifeee requested changes Aug 15, 2023

View reviewed changes

alifeee mentioned this pull request Aug 15, 2023

Add deprecation warning for colors changing from dict -> hex #1272

Closed

Use typing.Mapping for Python 3.8 Compatibility

24cd982

lavigne958 reviewed Aug 15, 2023

View reviewed changes

lavigne958 approved these changes Aug 16, 2023

View reviewed changes

Round color values in convert_hex_to_colors

f602d8d

Added rounding of the returned values in `convert_hex_to_colors` to 8 places after the decimal point. This matches the color values returned by the API. Updated values in `test_convert_hex_to_color`.

alifeee added this to the 6.0.0 milestone Aug 16, 2023

alifeee added a commit that referenced this pull request Aug 17, 2023

add hex <-> dict conversion for help

b22afa5

code by Ido Nechushtan <[email protected]> from #1270

alifeee mentioned this pull request Aug 18, 2023

Update readme with v6.0.0 migration guide #1279

Closed

Updated test_set_tab_color to compare hex values

58c4d1a

The test for `set_tab_color` has been updated to verify that the hex values of the colors returned from Google match the hex value given to the function. Removed rounding from (hex -> dict) utils function.

alifeee requested changes Aug 22, 2023

View reviewed changes

gspread/worksheet.py Outdated Show resolved Hide resolved

alifeee approved these changes Sep 3, 2023

View reviewed changes

alifeee requested a review from lavigne958 September 3, 2023 16:22

lavigne958 approved these changes Sep 10, 2023

View reviewed changes

lavigne958 merged commit 608789f into burnash:feature/release_6_0_0 Sep 10, 2023
5 checks passed

lavigne958 mentioned this pull request Sep 10, 2023

REFACTOR: Change how colors are handled internally (& externally) with update_tab_color #1213

Closed

alifeee mentioned this pull request Jan 26, 2024

make 6.0.0 release #1359

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement hex color conversion #1270

Implement hex color conversion #1270

idonec commented Aug 12, 2023

alifeee left a comment

alifeee commented Aug 13, 2023

idonec commented Aug 14, 2023

alifeee Aug 15, 2023

alifeee commented Aug 15, 2023

lavigne958 left a comment

lavigne958 Aug 15, 2023

alifeee commented Aug 16, 2023

alifeee commented Aug 16, 2023

alifeee commented Aug 16, 2023

lavigne958 commented Aug 19, 2023

alifeee left a comment

Implement hex color conversion #1270

Implement hex color conversion #1270

Conversation

idonec commented Aug 12, 2023

alifeee left a comment

Choose a reason for hiding this comment

alifeee commented Aug 13, 2023

idonec commented Aug 14, 2023

alifeee Aug 15, 2023

Choose a reason for hiding this comment

alifeee commented Aug 15, 2023

lavigne958 left a comment

Choose a reason for hiding this comment

lavigne958 Aug 15, 2023

Choose a reason for hiding this comment

alifeee commented Aug 16, 2023

alifeee commented Aug 16, 2023

alifeee commented Aug 16, 2023

lavigne958 commented Aug 19, 2023

alifeee left a comment

Choose a reason for hiding this comment