rcgen icon indicating copy to clipboard operation
rcgen copied to clipboard

Consider eagerly implementing common traits

Open lvkv opened this issue 7 months ago • 5 comments

My immediate need/use case is Clone on CertificateSigningRequest. After skimming the codebase, however, I'm getting the impression that there are many more opportunities to implement the common traits outlined in the API guidelines. @est31 also echoed this over at https://github.com/rustls/rcgen/issues/17#issuecomment-503656409.

Prior related work:

  • https://github.com/rustls/rcgen/issues/315 / https://github.com/rustls/rcgen/pull/316
  • https://github.com/rustls/rcgen/pull/319
  • and more, probably...

If you're amenable to eagerly implementing these traits, I think it'd be easiest (from a review standpoint) to create a pull request per trait and dedicate each PR to sweeping the codebase and nitpicking the details—similar to https://github.com/rustls/rcgen/pull/316. I'm open to other approaches too.

  • [x] Copy, Clone
    • [x] https://github.com/rustls/rcgen/pull/341
  • [ ] Debug
    • [x] https://github.com/rustls/rcgen/pull/316
    • [ ] https://github.com/rustls/rcgen/pull/343
  • [x] Display
    • [x] (Deemed not necessary for now)
  • [ ] PartialEq, Eq
    • [ ] https://github.com/rustls/rcgen/pull/344
  • [x] PartialOrd, Ord
    • [x] (Closed) https://github.com/rustls/rcgen/pull/346
  • [x] Hash
    • [x] (Closed) https://github.com/rustls/rcgen/pull/345
  • [x] Default
    • [x] (Deemed not necessary for now)

lvkv avatar May 14 '25 21:05 lvkv

After skimming the codebase, however, I'm getting the impression that there are many more opportunities to implement the common traits outlined in the API guidelines.

Agreed! I think some of these were harder to take back when some of the params types were holding private data but we're in a better spot now.

I think it'd be easiest (from a review standpoint) to create a pull request per trait and dedicate each PR to sweeping the codebase and nitpicking the details

I'd also be open to one PR with each trait handled per-commit but don't feel strongly.

cpu avatar May 14 '25 23:05 cpu

One trait per commit in a single PR sounds good to me.

djc avatar May 15 '25 05:05 djc

#341 was pretty seamless, so I'll open one more PR for all the other common traits—one trait per commit.

lvkv avatar May 15 '25 22:05 lvkv

I think we should stop after the PRs that already exist and wait for someone who has a particular impl they require.

djc avatar May 19 '25 10:05 djc

I think we should stop after the PRs that already exist

Feel free to manage/close this PR whenever, by the way. There are no further requests on my end after https://github.com/rustls/rcgen/pull/341 🙂.

lvkv avatar May 19 '25 19:05 lvkv

@lvkv I think you check off all items on the list and close this issue now as all PRs related to your list have been merged into main or closed with a reason.

Rynibami avatar Jun 20 '25 11:06 Rynibami

@Rynibami @lvkv Thanks to both of you for your work here. I'll close this as completed.

cpu avatar Jun 20 '25 13:06 cpu