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

Do not raise a DeprecationWarning in stripe.app_info #1168

Merged
merged 1 commit into from
Dec 11, 2023

Conversation

akx
Copy link
Contributor

@akx akx commented Dec 11, 2023

Fixes #1167.

Regressed in 0c7ad96 (#1153).

@CLAassistant
Copy link

CLAassistant commented Dec 11, 2023

CLA assistant check
All committers have signed the CLA.

@akx
Copy link
Contributor Author

akx commented Dec 11, 2023

cc @pakrym-stripe

@pakrym-stripe
Copy link
Contributor

I'm afraid we need that line for the reason the comment outlined.
Will wrapping that import with

        with warnings.catch_warnings():
            warnings.filterwarnings("ignore", category=DeprecationWarning)

work?

@akx
Copy link
Contributor Author

akx commented Dec 11, 2023

@pakrym-stripe What is the late import the comment refers to? I tried figuring it out, but no luck.

As I see it, the line

import stripe.app_info

ends up importing app_info, so app_info is <module 'stripe.app_info' from '/usr/local/lib/python3.12/site-packages/stripe/app_info.py'> after that (and a deprecation warning was emitted).

Then, the name is overwritten with None some lines later:

app_info: Optional[AppInfo] = None

@pakrym-stripe
Copy link
Contributor

pakrym-stripe commented Dec 11, 2023

Yes, and app_info variable is used for a different purpose. If you don't import app_info module early, the late import will overwrite the stripe.app_info. This was a naming mistake on our part, unfortunately.

@akx
Copy link
Contributor Author

akx commented Dec 11, 2023

I'm still not following the terminology "late import" here, unless you mean this corner-case situation (which is not really what I'd call a late import), when an user imports stripe, then separately does import stripe.app_info and not from stripe import app_info (note how the import order has an effect too):

(no-deprecation-warning-on-import) $ python -c 'import stripe; import stripe.app_info as ai; from stripe import app_info as ai2; print(ai, ai2)'
<module 'stripe.app_info'> <module 'stripe.app_info'>
(no-deprecation-warning-on-import) $ python -c 'import stripe; from stripe import app_info as ai2; import stripe.app_info as ai; print(ai, ai2)'
<module 'stripe.app_info'> None
(master) $ python -c 'import stripe; import stripe.app_info as ai; from stripe import app_info as ai2; print(ai, ai2)'
None None
(master) $ python -c 'import stripe; from stripe import app_info as ai2; import stripe.app_info as ai; print(ai, ai2)'
None None

(Across all of GitHub, there seems to be exactly one place that does import stripe.app_info: this repository and this line of code.)

@akx
Copy link
Contributor Author

akx commented Dec 11, 2023

Also, with the current setup there's actually no point in stripe.app_info even having the DeprecationWarning in the first place; since it's implicitly imported by import stripe, trying to re-import it would not emit the warning again (because the module will have already been executed).

On that note, if you like, @pakrym-stripe, I can rework this PR to just remove the DeprecationWarning from there.

@pakrym-stripe
Copy link
Contributor

pakrym-stripe commented Dec 11, 2023

unless you mean this corner-case situation (which is not really what I'd call a late import), when an user imports stripe, then separately does import stripe.app_info

Yep, that.

python -c 'import stripe; import stripe.app_info; print(stripe.app_info)'
<module 'stripe.app_info' from '/Users/pakrym/stripe/stripe-python/stripe/app_info.py'>

If stripe.app_info if not eagerly imported in __init__, the separate import of stripe.app_info will overwrite stripe.app_info.

Also, with the current setup there's actually no point in stripe.app_info even having the DeprecationWarning in the first place; since it's implicitly imported by import stripe, trying to re-import it would not emit the warning again (because the module will have already been executed).

You are right here, but we'd like to keep all backward compatibility re-exporting files uniform. We also have have a linter that depend on the warning being there (https://github.com/stripe/stripe-python/blob/master/flake8_stripe/flake8_stripe.py#L136)

@akx
Copy link
Contributor Author

akx commented Dec 11, 2023

I think the suggested warnings.catch_warnings() filter thing is a bit of a silly waste of users' cycles (especially since it's not thread-safe, etc., etc.), if the simplest option is to get rid of the deprecation warning (with a suitable comment explaining why it's not there) and adding an exemption for that file – after all, there are per-file-ignores for IMP102s already.

@pakrym-stripe
Copy link
Contributor

Alright, but please leave the

  The stripe.app_info package is deprecated, please change your
    imports to import from stripe directly.
    From:
      from stripe.app_info import AppInfo
    To:
      from stripe import AppInfo

part as a comment

@akx akx force-pushed the no-deprecation-warning-on-import branch from 2ad13f1 to 980d4b5 Compare December 11, 2023 17:49
@akx akx changed the title Do not needlessly import stripe.app_info Do not raise a DeprecationWarning in stripe.app_info Dec 11, 2023
@akx
Copy link
Contributor Author

akx commented Dec 11, 2023

@pakrym-stripe Okiedoke, changed. Flake8 seems to be happy locally too.

@pakrym-stripe pakrym-stripe merged commit e99ae54 into stripe:master Dec 11, 2023
15 checks passed
@pakrym-stripe
Copy link
Contributor

Great work! Thank you for your help @akx !

@akx akx deleted the no-deprecation-warning-on-import branch December 12, 2023 13:52
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.

Importing stripe==7.8.1 raises a deprecation warning
3 participants