Skip to content

Fix ValueError in get_graph_kwargs#14022

Merged
bryevdv merged 2 commits intobokeh:branch-3.6from
cdeil:get_graph_kwargs_valueerror_fix
Aug 14, 2024
Merged

Fix ValueError in get_graph_kwargs#14022
bryevdv merged 2 commits intobokeh:branch-3.6from
cdeil:get_graph_kwargs_valueerror_fix

Conversation

@cdeil
Copy link
Contributor

@cdeil cdeil commented Aug 11, 2024

This PR fixes the warning I see in PyCharm static code analysis:

Unresolved attribute reference 'message' for class 'ValueError'

This seems to work:

In [6]: import sys

In [7]: node_source = "my node source"

In [8]: try:
   ...:     raise ValueError("spam alot")
   ...: except ValueError as err:
   ...:     msg = f"Failed to auto-convert {type(node_source)} to ColumnDataSource.\n Original error: {err.message}"
   ...:     raise ValueError(msg).with_traceback(sys.exc_info()[2])
   ...: 
---------------------------------------------------------------------------
ValueError                                Traceback (most recent call last)
Cell In[8], line 2
      1 try:
----> 2     raise ValueError("spam alot")
      3 except ValueError as err:

ValueError: spam alot

During handling of the above exception, another exception occurred:

AttributeError                            Traceback (most recent call last)
Cell In[8], line 4
      2     raise ValueError("spam alot")
      3 except ValueError as err:
----> 4     msg = f"Failed to auto-convert {type(node_source)} to ColumnDataSource.\n Original error: {err.message}"
      5     raise ValueError(msg).with_traceback(sys.exc_info()[2])

AttributeError: 'ValueError' object has no attribute 'message'

In [9]: try:
   ...:     raise ValueError("spam alot")
   ...: except ValueError as err:
   ...:     msg = f"Failed to auto-convert {type(node_source)} to ColumnDataSource.\n Original error: {err}"
   ...:     raise ValueError(msg).with_traceback(sys.exc_info()[2])
   ...: 
---------------------------------------------------------------------------
ValueError                                Traceback (most recent call last)
Cell In[9], line 2
      1 try:
----> 2     raise ValueError("spam alot")
      3 except ValueError as err:

ValueError: spam alot

During handling of the above exception, another exception occurred:

ValueError                                Traceback (most recent call last)
Cell In[9], line 5
      3 except ValueError as err:
      4     msg = f"Failed to auto-convert {type(node_source)} to ColumnDataSource.\n Original error: {err}"
----> 5     raise ValueError(msg).with_traceback(sys.exc_info()[2])

Cell In[9], line 2
      1 try:
----> 2     raise ValueError("spam alot")
      3 except ValueError as err:
      4     msg = f"Failed to auto-convert {type(node_source)} to ColumnDataSource.\n Original error: {err}"

ValueError: Failed to auto-convert <class 'str'> to ColumnDataSource.
 Original error: spam alot

Is it really useful to catch a ValueError and raise another?
Apply this fix here or instead remove the whole try/except?

@codecov
Copy link

codecov bot commented Aug 11, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Please upload report for BASE (branch-3.6@5aecc61). Learn more about missing BASE report.

Additional details and impacted files
@@              Coverage Diff              @@
##             branch-3.6   #14022   +/-   ##
=============================================
  Coverage              ?   93.16%           
=============================================
  Files                 ?      282           
  Lines                 ?    19850           
  Branches              ?        0           
=============================================
  Hits                  ?    18494           
  Misses                ?     1356           
  Partials              ?        0           

@bryevdv
Copy link
Member

bryevdv commented Aug 11, 2024

Is it really useful to catch a ValueError and raise another?

The value is in providing a more tailored exception message with more information for the user.

Similar to the other PR it would be good to add some tests for these codepaths since evidently they were missing.

@bryevdv bryevdv added status: WIP type: bug python Issues that should only require updating Python code labels Aug 11, 2024
@bryevdv bryevdv added this to the 3.6 milestone Aug 11, 2024
@cdeil
Copy link
Contributor Author

cdeil commented Aug 14, 2024

Added a test: 079b336

@bryevdv
Copy link
Member

bryevdv commented Aug 14, 2024

Thanks @cdeil !

@bryevdv bryevdv merged commit f6ded42 into bokeh:branch-3.6 Aug 14, 2024
@mattpap mattpap mentioned this pull request Aug 22, 2024
12 tasks
mattpap pushed a commit that referenced this pull request Aug 22, 2024
* Fix ValueError in get_graph_kwargs

* Add Test_get_graph_kwargs::test_bad_input
@mattpap mattpap modified the milestones: 3.6, 3.5.2 Aug 22, 2024
Chiemezuo pushed a commit to Chiemezuo/bokeh that referenced this pull request Aug 27, 2024
* Fix ValueError in get_graph_kwargs

* Add Test_get_graph_kwargs::test_bad_input
@github-actions
Copy link

github-actions bot commented Dec 4, 2024

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 4, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

python Issues that should only require updating Python code reso: completed status: accepted type: bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants