Skip to content

Conversation

@jvanasco
Copy link
Contributor

@jvanasco jvanasco commented Dec 21, 2024

This is the requested alternative to #196 ; while I was happy to generate this PR, I would rather see a less breaking approach as suggested in the other PR.

The only other thing to note here is changing the test case that proxies attributes from the wrapped object, as Cryptography does not have that particular attribute. In the other PR I calculated it; in this one I just switched to another one. IMHO, I think entirely dropping the attribute proxy is worth considering.

Edit: The approach in #182 is possibly better than this one as it fully drops the ComparableX509 object. If things are going to break, they might as well drop that as it exists to only compare two objects.

@ohemorange
Copy link
Contributor

Thanks for writing up this alternative! I agree that just going ahead and dropping ComparableX509 makes sense; do you think it would be cleaner to do that here or in a separate PR? Not sure which attribute proxying you're referring to the in comment, but seems like it would probably address that as well.

@ohemorange
Copy link
Contributor

josepy 1.15.0 has released, so we're now ready to start integrating this code! I do think ComparableX509 should just be dropped completely; are you down to modify this PR or would you rather I make a new one? I know you've done a bunch of work on this and want to give you the chance to get that acknowledged in the history.

@jvanasco
Copy link
Contributor Author

Thanks for reaching out. I'd be happy to get you a PR in the morning.

@jvanasco jvanasco mentioned this pull request Jan 23, 2025
@jvanasco
Copy link
Contributor Author

Closed. Addressed in #212

@jvanasco jvanasco closed this Jan 31, 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