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 warning to Function Trait Type #1397

Merged
merged 4 commits into from
Jan 15, 2021

Conversation

aaronayres35
Copy link
Contributor

@aaronayres35 aaronayres35 commented Jan 14, 2021

Fixes #1215
I can open a separate issue about fixing up the Method Trait Type (see #1215 (comment)) if this PR is merged.

This issue was not on the 6.2 milestone, and if we do not want to add this before the 6.2 release I am happy to delay this PR.

This PR declares the Function trait type as deprecated in its doc string, but does not actually raise a deprecation warning yet. It also adds a footnote mentioning the deprecation/linking to the api docs on the "Defining Traits" page in the table of predefined traits next to Function.

Checklist

  • Tests
  • Update API reference (docs/source/traits_api_reference)
  • Update User manual (docs/source/traits_user_manual)
  • [ ] Update type annotation hints in traits-stubs

Copy link
Member

@mdickinson mdickinson left a comment

Choose a reason for hiding this comment

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

Thanks for this. The general shape of the changes looks good. I think we should go one way or the other, though - either bite the bullet and actually make Function deprecated, or limit ourselves to offering advice on using Callable instead, and not actually say that the class is deprecated.

I did find a number of uses of Function in existing (including current) projects, so actual deprecation would be somewhat disruptive.

So I'd suggest that we go with what's here, except that we:

  • remove the .. deprecated directive (but keep the contents of that directive; maybe change .. deprecated to .. note)
  • keep the Function [3] note, but change it to say that in most cases Callable should be used instead

As a compromise, and to motivate people to move away from Function, we could also say either in the docstring, or in the note (or both) that the Function trait type "may be deprecated in the future".

@mdickinson
Copy link
Member

Okay, looking harder, I'm changing my mind again. (Sorry.) Let's be bold and actually deprecate this beast.

I'll contact directly the projects that are using Function to give them some warning.

@aaronayres35 Would you be able to add actual deprecation and tests for this PR?

@aaronayres35
Copy link
Contributor Author

@aaronayres35 Would you be able to add actual deprecation and tests for this PR?

Sure thing!

@aaronayres35 aaronayres35 changed the title Add soft deprecation warning to Function Trait Type docstring Add deprecation warning to Function Trait Type docstring Jan 15, 2021
Copy link
Member

@mdickinson mdickinson left a comment

Choose a reason for hiding this comment

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

LGTM

@mdickinson
Copy link
Member

I can open a separate issue about fixing up the Method Trait Type

Yes, please! That one may be even easier to deprecate; I suspect we have close to zero uses of it out in the wild.

@aaronayres35 aaronayres35 merged commit 7638180 into master Jan 15, 2021
@aaronayres35 aaronayres35 deleted the discourage-function branch January 15, 2021 15:18
@aaronayres35 aaronayres35 changed the title Add deprecation warning to Function Trait Type docstring Add deprecation warning to Function Trait Type Jan 15, 2021
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.

Discourage uses of the Function trait type
2 participants