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

Logging future warning #65

Merged

Conversation

tonykploomber
Copy link
Contributor

@tonykploomber tonykploomber commented Jun 1, 2023

closes #50

Changes:

  • Add deprecation_warning func
  • Add PR Template

📚 Documentation preview 📚: https://ploomber-core--65.org.readthedocs.build/en/65/

@tonykploomber
Copy link
Contributor Author

tonykploomber commented Jun 1, 2023

@edublancas
In the new change, I propose to make deprecation_warning accept the telemetry instance (optional)
Example, in the jupysql we can use:

from sql.telemetry import telemetry

deprecation_warning("you are using old feature", telemetry)

This is because we want to record the module (jupysql, ploomber...), which invokes the deprecation_warning.
Major benefit is we can see the different module

If we create the telemetry instance from ploomber-core and use it, we will hard to see which module invokes thedeprecation_warning, so I think take existing telemetry instance from modules is better idea

On posthug it will look like:
Screenshot 2023-06-01 at 4 24 44 PM

@tonykploomber tonykploomber changed the title Tonyk/logging future warning 2 Logging future warning Jun 1, 2023
@edublancas edublancas requested a review from neelasha23 June 2, 2023 02:39
@neelasha23
Copy link
Contributor

Do we need to add changelog entry? looks good besides this!

neelasha23
neelasha23 previously approved these changes Jun 5, 2023
Copy link
Contributor

@edublancas edublancas left a comment

Choose a reason for hiding this comment

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

please document this new approach here: https://ploomber-core.readthedocs.io/en/latest/deprecation.html

and add some context (the context is described in the original issue, which explains why we need this vs the existing deprecated module)

@tonykploomber tonykploomber requested a review from edublancas June 6, 2023 21:32
@edublancas edublancas merged commit f8dc0cf into ploomber:main Jun 7, 2023
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.

logging future warnings
3 participants