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: Display deprecated warnings for mapbox traces #4900

Merged
merged 8 commits into from
Nov 25, 2024

Conversation

archmoj
Copy link
Contributor

@archmoj archmoj commented Nov 22, 2024

Closes #4730.

Adds deprecation warnings which are printed to the console when using one of the following objects / functions:

graph objects:

  • go.Choroplethmapbox
  • go.Densitymapbox
  • go.Scattermapbox

express:

  • px.choropleth_mapbox
  • px.density_mapbox
  • px.scatter_mapbox
  • px.line_mapbox

Since the graph objects files are generated by codegen, this PR also modifies the codegen process to automatically add the deprecation warnings to the generated files.

@archmoj archmoj requested a review from emilykl November 22, 2024 11:24
@emilykl emilykl changed the title Fix: Display deprecated warnings for mapbox traces and skip their template validation Fix: Display deprecated warnings for mapbox traces Nov 25, 2024
@emilykl
Copy link
Contributor

emilykl commented Nov 25, 2024

@LiamConnors When you have a chance, could you sanity check whether the warnings are working as expected for you on this branch? I.e. that we see a warning when using the deprecated traces, but no warnings otherwise.

@LiamConnors
Copy link
Member

It's working as expected. Looks good to me.

@emilykl
Copy link
Contributor

emilykl commented Nov 25, 2024

Thanks Liam. @archmoj @gvwilson This is ready for review.

Copy link
Contributor

@gvwilson gvwilson left a comment

Choose a reason for hiding this comment

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

thank you

+ " Use *{node.name_property.replace("mapbox", "map")}* instead."
+ " Learn more at: https://plotly.com/python/mapbox-to-maplibre/",
stacklevel=2,
category=DeprecationWarning,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@emilykl Good improvement. And I can approve this PR.
As follow ups to this PR:
Could you possibly

  1. double check that stacklevel=2 is the best option we could use?
  2. should we use similar stacklevel and category options for other warnings in plotly.py?
    Thank you!

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe stacklevel=2 is the correct option in this case -- it causes the warning to appear to be emitted at the place in the user's code where they have called the deprecated function, for example:

Making a go.Choroplethmapbox figure...
/Users/ekl/code/plotly.py/my_script.py:32: DeprecationWarning:

*choroplethmapbox* is deprecated! Use *choroplethmap* instead. Learn more at: https://plotly.com/python/mapbox-to-maplibre/

(Also, for DeprecationWarning specifically, using any other stacklevel for these deprecations causes the warning to not appear at all.)

It's just coincidence that stacklevel=2 is the correct option in this case for both the go and px deprecations... the correct stacklevel will differ in every case depending on exactly where the warning is being emitted.

I don't think it's worth the effort to go through the codebase and clean up existing warnings, but I do think it makes sense to use stacklevel and category consistently going forward. It seems at least some of the warnings already in place in Plotly.py are using them correctly.

@archmoj archmoj merged commit 4a21e76 into master Nov 25, 2024
5 checks passed
@archmoj archmoj deleted the v6-mapbox-deprecated-warning branch November 25, 2024 17:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add deprecation warning for Mapbox traces
4 participants