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

Use create() rather than _create() #1215

Merged
merged 7 commits into from
Mar 14, 2023
Merged

Use create() rather than _create() #1215

merged 7 commits into from
Mar 14, 2023

Conversation

corranwebster
Copy link
Contributor

@corranwebster corranwebster commented Mar 11, 2023

This carries out the next two steps of #729

  • replaces calls to _create with calls to create
  • that classes migrate the implementation of _create into create, and make _create call create

as well as the next step in removing create calls from __init__ methods:

  • properly deprecate auto-create, but don't fail yet; push toward removing create keyword argument.

@mdickinson mdickinson self-requested a review March 13, 2023 16:58
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; a few comments.

This PR seems to include a couple of .zip files that I think shouldn't be there. (cf: #776)

PendingDeprecationWarning,
"in a future Pyface version, code should not pass the create "
"parameter and should instead call create() explicitly",
DeprecationWarning,
Copy link
Member

Choose a reason for hiding this comment

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

It may be worth adding a suitable stacklevel parameter here so that the warning message points to the client code (i.e., the code that's passing the create parameter).

Also applies to other uses of DeprecationWarning in this PR.

warn(
"The _create() method will be removed in a future version of "
"Pyface. Use create() instead.",
PendingDeprecationWarning,
Copy link
Member

Choose a reason for hiding this comment

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

OT: I'm still not sure I understand the use-cases for PendingDeprecationWarning; it's tempting just to avoid it and use DeprecationWarning everywhere instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In my mind the difference is between "this is going to go away; but we're still using it internally" vs "this is going to go away; and we aren't using it internally any more". That allows two levels of warning for downstream projects.

But yes, this should probably be a deprecation warning.

Copy link
Member

@mdickinson mdickinson Mar 14, 2023

Choose a reason for hiding this comment

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

I suspect this addition was unintentional.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, sorry; I see it's a modification, not an addition.

Copy link
Member

Choose a reason for hiding this comment

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

Unintentional addition?

@corranwebster
Copy link
Contributor Author

This PR seems to include a couple of .zip files that I think shouldn't be there. (cf: #776)

That's what I get for doing git commit -a after a search and replace :(

@mdickinson
Copy link
Member

Yes, sorry - I take it back about the .zip files. I thought they were new files, but they're modifications to existing files.

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.

2 participants