Skip to content

deprecate comparablex509#207

Merged
ohemorange merged 3 commits intomainfrom
deprecate-comparablex509
Jan 17, 2025
Merged

deprecate comparablex509#207
ohemorange merged 3 commits intomainfrom
deprecate-comparablex509

Conversation

@bmw
Copy link
Member

@bmw bmw commented Jan 17, 2025

this is an alternate to #206

one thing i noticed is that all code deprecated in #206 uses ComparableX509 in some way so maybe we could just put this warning in one spot. i also expanded the warning text a bit and put something about it in the changelog

initially i also wanted to make use of code like https://github.com/certbot/josepy/blob/main/src/josepy/util.py#L259-L293 so users who have code like from josepy.util import ComparableX509 get a warning about that problematic import as well, but it's kind of annoying because we also reexport ComparableX509 in __init__.py. we could also do the same thing there, but it gets a little messy because with a naive approach __init__.py would be generating its own warning for importing josepy.util.ComparableX509 on top of generating another warning if anyone touches the reexported attribute. i personally think all that is probably too much to be worth the effort and this warning in ComparableX509.__init__ is good enough

erica, feel free to suggest changes or update one of your own PRs with proposed alternatives

@bmw bmw requested a review from ohemorange January 17, 2025 01:10
Copy link
Contributor

@ohemorange ohemorange left a comment

Choose a reason for hiding this comment

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

I like this approach better! I agree it's not worth it to warn on import, this seems fine to me. I did just remember though that sphinx has a deprecated tag, which we should add.

Co-authored-by: ohemorange <ebportnoy@gmail.com>
@ohemorange ohemorange merged commit 49ae1fe into main Jan 17, 2025
@ohemorange ohemorange deleted the deprecate-comparablex509 branch January 17, 2025 17:35
@jvanasco jvanasco mentioned this pull request Jan 23, 2025
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