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.

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

Fix ValueError in get_graph_kwargs #14022

Merged
merged 2 commits into from
Aug 14, 2024

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?

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
24 checks passed
@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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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