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

Add deprecation message to CallDefault function partly resolves issue#6257 #6901

Merged
merged 5 commits into from
May 16, 2024

Conversation

MarchisLost
Copy link
Contributor

@MarchisLost MarchisLost commented May 7, 2024

References and relevant issues

Partly closes #6257 by adding a deprecation message to the CallDefault function that seems to not be used since #2266

Description

  • - My PR is the minimum possible work for the desired functionality
  • - I have commented my code, particularly in hard-to-understand areas
  • - I have made corresponding changes to docstrings and documentation (open a PR on the docs repository (https://github.com/napari/docs) if relevant!)
  • - I have added tests that prove my fix is effective or that my feature works
  • - If I included new strings, I have used trans._("some string") to make them localizable.
    (For more information see our translations guide).

@melissawm melissawm added the deprecation Adds a deprecation warning label May 7, 2024
@melissawm
Copy link
Member

Hi @MarchisLost - thanks for your contribution!

Would you mind changing the title of your PR to be more descriptive of the problem you are solving? This will help your PR get more visibility and will help the maintainers understand your work better. In your case, I would suggest something like "Add deprecation message to CallDefault function" or something similar.

Welcome and let us know if you have questions 😄

@MarchisLost MarchisLost changed the title Issue#6257 Add deprecation message to CallDefault function partly resolves issue#6257 May 7, 2024
@MarchisLost
Copy link
Contributor Author

Hi, thank you for the feedback.
I have changed the PR title.

Copy link

codecov bot commented May 7, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.43%. Comparing base (4a56237) to head (808c1ed).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6901      +/-   ##
==========================================
- Coverage   92.48%   92.43%   -0.06%     
==========================================
  Files         614      614              
  Lines       55181    55182       +1     
==========================================
- Hits        51035    51008      -27     
- Misses       4146     4174      +28     

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

@DragaDoncila
Copy link
Contributor

Hi @MarchisLost thanks for the PR! I'm pinging @melonora for review, because I think this is similar to #6836 that he recently looked at

Comment on lines 401 to 406
warnings.warn(
trans._(
'`CallDefault` is deprecated since 0.4.6 and will be removed in the future',
),
category=DeprecationWarning,
)
Copy link
Contributor

@melonora melonora May 15, 2024

Choose a reason for hiding this comment

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

Suggested change
warnings.warn(
trans._(
'`CallDefault` is deprecated since 0.4.6 and will be removed in the future',
),
category=DeprecationWarning,
)
warnings.warn(
trans._(
'`CallDefault` is deprecated in napari v0.5.0 and will be removed in v0.6.0.',
),
category=DeprecationWarning,
)

Copy link
Contributor

@melonora melonora left a comment

Choose a reason for hiding this comment

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

Thanks @MarchisLost (interesting name haha) and welcome to napari! I have checked and indeed this does not seem to be used at all. However, for good practice we have 1 release always for warning that it is deprecated and then the release after that we remove. If we slightly change the message we are good to go for merging this one. Once you have accepted the suggestion I will mark your PR ready for merge and then if nobody else has obligations we merge within a day.

@melonora
Copy link
Contributor

Approved! Thanks:)

@melonora melonora added the ready to merge Last chance for comments! Will be merged in ~24h label May 15, 2024
@MarchisLost
Copy link
Contributor Author

Hi @melonora thank you!

@melonora melonora added the maintenance PR with maintance changes, label May 15, 2024
@melonora melonora added this to the 0.5.0 milestone May 16, 2024
@melonora
Copy link
Contributor

in it goes!

@melonora melonora merged commit 9836ed1 into napari:main May 16, 2024
34 checks passed
@github-actions github-actions bot removed the ready to merge Last chance for comments! Will be merged in ~24h label May 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deprecation Adds a deprecation warning maintenance PR with maintance changes,
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Deprecate/Remove CallDefault
4 participants