Skip to content

Conversation

@sstephanyy
Copy link
Contributor

@sstephanyy sstephanyy commented Oct 27, 2024

Description

  • Deprecation of "default" colorscale: Updated ridgeplot._ridgeplot::ridgeplot() to raise a deprecation warning when the "default" value is passed to the colorscale argument. The warning informs users that this value is deprecated and directs them topx.colors.named_colorscales() in Plotly Express to view valid colorscale names. More details can be found in the Plotly documentation.

  • Update list_all_colorscale_names(): Added a deprecation warning to list_all_colorscale_names(), advising users to use px.colors.named_colorscales() from Plotly Express instead. The docstring has also been updated with .. deprecated:: 0.1.31 and links to the relevant documentation.

  • Tests: Added tests to verify the deprecation warnings in both functions.

Related issues

Closes #256

Question

I noticed there's already a 'Deprecations' section in docs/reference/changelog.md for notifying users about deprecation warnings. Should I add a note there that the list_all_colorscale_names() function will be removed soon and that "deafult" will be deprecated?

PR check list

  • Read the contributing guidelines.
    • You don't have to read the whole thing, but it's a good idea to skim it. And definitely take a look at it if you're experiencing any issues getting your local environment up and running.
  • Provided the relevant details in the PR's description.
  • Referenced relevant issues in the PR's description.
  • Added tests for all my changes.
    • The CI will fail unless 100% of all new code is covered by the tests.
  • Updated the docs for relevant changes.
    • New modules (or renamed ones) are included in docs/api/internal/.
    • New public functions/classes/variables are documented in docs/api/index.rst.
    • Added the appropriate versionadded, versionchanged, or deprecated directives to docstrings.
      • The version should be the next release version, which you can infer by bumping the minor version in MAJOR.MINOR.PATCH (e.g., if the current version is 0.2.3, the next release will be 0.3.0).
  • Changes are documented in docs/reference/changelog.md.
    • Please try to follow the conventions laid out in the file. In doubt, just ask!
  • Consider granting push permissions to your PR's branch, so maintainers can help you out if needed.
  • The CI check are all passing, or I'm working on fixing them!
  • I have reviewed my own code! 🤠

📚 Documentation preview 📚: https://ridgeplot--262.org.readthedocs.build/en/262/

Copy link

@github-actions github-actions bot 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 for submitting your first pull request with us! 🎉

Our response times may vary, but we'll get back to you as soon as we can!

To help us help you, please make sure you have ticked all the boxes in the pull request template.

Welcome aboard! 🚀

@tpvasconcelos
Copy link
Owner

@sstephanyy Congrats (and thank you!) for your first contribution!!

This is pretty much exactly how I would've approached it so I consider it ready to merge, but I will take a better look at it tomorrow 👍

@codecov
Copy link

codecov bot commented Oct 27, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (7568f28) to head (1439ed2).

Additional details and impacted files
@@            Coverage Diff            @@
##              main      #262   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           13        13           
  Lines          528       532    +4     
  Branches        69        70    +1     
=========================================
+ Hits           528       532    +4     
Flag Coverage Δ
combined-src 99.24% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@tpvasconcelos
Copy link
Owner

@sstephanyy there was an issue with how the documentation-links job was defined. Sorry about that.

btw, I also noticed that the tests were not passing locally but I fixed the issue in this commit: cb105e6

@tpvasconcelos
Copy link
Owner

There seem to be some other related issues with the Codacy and Codecov integrations... Sorry about this as well. I'll try to take a look tomorrow

tpvasconcelos
tpvasconcelos previously approved these changes Oct 28, 2024
Copy link
Owner

@tpvasconcelos tpvasconcelos left a comment

Choose a reason for hiding this comment

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

Awesome work! 🚀🚀🚀

I had to tweak the tests a bit here because they were failing, but the remaining changes were all cosmetic.

@tpvasconcelos
Copy link
Owner

The only thing left for you to do is to update the changelog with a small entry, like:

Deprecated colorscale='default' and list_all_colorscale_names() in favour or Plotly Express' px.colors.named_colorscales()

@tpvasconcelos
Copy link
Owner

I noticed there's already a 'Deprecations' section in docs/reference/changelog.md for notifying users about deprecation warnings. Should I add a note there that the list_all_colorscale_names() function will be removed soon and that "deafult" will be deprecated?

Yes, you can add it as a new item under the latest ### Deprecations section 👍

@sstephanyy
Copy link
Contributor Author

The only thing left for you to do is to update the changelog with a small entry, like:

Deprecated colorscale='default' and list_all_colorscale_names() in favour or Plotly Express' px.colors.named_colorscales()

Done ✔️

@tpvasconcelos
Copy link
Owner

@sstephanyy FYI: f464009 😉

@sstephanyy
Copy link
Contributor Author

@sstephanyy FYI: f464009 😉

Thank you so much haha! I’m really glad to have contributed to the project. 😉

Copy link
Owner

@tpvasconcelos tpvasconcelos left a comment

Choose a reason for hiding this comment

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

🚀🚀🚀🚀🚀🚀🚀🚀🚀🚀🚀🚀🚀🚀🚀🚀🚀🚀

@tpvasconcelos tpvasconcelos merged commit 54f22df into tpvasconcelos:main Oct 28, 2024
@sstephanyy
Copy link
Contributor Author

🚀🚀🚀🚀🚀🚀🚀🚀🚀🚀🚀🚀🚀🚀🚀🚀🚀🚀

Please let me know if there's anything else I can help with.

@tpvasconcelos
Copy link
Owner

Thanks for your help @sstephanyy ! 🚀

Your contribution has now been merged to the main branch and will be part of our first minor release (0.2.0). You can track the progress here: https://github.com/tpvasconcelos/ridgeplot/milestone/1

Btw, since you voiced some interested the other day, I think this could be a nice issue to try and tackle (when you have the time, no rush!) --> #252

@sstephanyy sstephanyy deleted the chore/deprecate-default-colorscale branch October 30, 2024 23:18
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.

Deprecate the "default" colorscale name and list_all_colorscale_names()

2 participants