Missing fmt::Debug impls for public structs and enums #735
Conversation
There was a problem hiding this comment.
LGTM, but like the other PR, my sense is that some of these impls aren't needed to pass the lint? Or does the lint apply to internal types too?
The reason why I'm being a stickler about this is that I'd rather not add impls for things that we don't need because they likely increase compile times. And the compile times for this crate are already pretty poor.
src/re_trait.rs
Outdated
| .field("last_match", &self.last_match) | ||
| .finish() | ||
| } | ||
| } |
There was a problem hiding this comment.
I suspect you're writing out this impl because Matches is generic and doesn't require its type parameters to impl Debug? The RegularExpression trait is an internal construct, so adding a requirement to impl Debug seems fine to me. (For both Self and for its Text associated type.) Then you should be able to derive Debug impls here.
There was a problem hiding this comment.
I also made the same change to CaptureMatches.
src/exec.rs
Outdated
| @@ -97,6 +99,7 @@ struct ExecReadOnly { | |||
| /// Facilitates the construction of an executor by exposing various knobs | |||
| /// to control how a regex is executed and what kinds of resources it's | |||
| /// permitted to use. | |||
| #[derive(Debug)] | |||
There was a problem hiding this comment.
Is this Debug impl necessary?
There was a problem hiding this comment.
ExecBuilder is public via the internal module which "exists to support suspicious activity".
I'll remove the debug impl and allow the lint.
src/compile.rs
Outdated
| @@ -25,6 +26,7 @@ struct Patch { | |||
|
|
|||
| /// A compiler translates a regular expression AST to a sequence of | |||
| /// instructions. The sequence of instructions represents an NFA. | |||
| #[derive(Debug)] | |||
There was a problem hiding this comment.
Is this Debug impl necessary?
There was a problem hiding this comment.
Compiler is public via the internal module which "exists to support suspicious activity".
I'll remove the debug impl and allow the lint.
There was a problem hiding this comment.
Haha right. Yeah, it's technically public, but is doc(hidden) and not a stable part of the API. I'm hoping to remove that quirk in the move to regex-automata.
Thanks.
|
@BurntSushi I've made the requested changes. I squashed these into the commit for the For types that are only public via the hidden |
|
rebase incoming. |
For reference, the other PR is #734. |
|
Whoops looks like I accidentally didn't split out the iterator traits bits from Thanks for merging. Would it be possible to get these two PRs packaged into a release soon? |
|
Yeah I saw that, but that's okay. No biggie. I'll try to do a release in the next couple days. Please ping me again if it doesn't happen. |
|
hi @BurntSushi following up with a friendly ping for a release. |
|
@lopopolo OK, |
|
Thank you! |
rust-lang/regex#735 added missing `fmt::Debug` impls for public structs and enums, which `spinoso-regexp` requires to impl `fmt::Debug` on its own iterator types. This change does not use these `fmt::Debug` impls, just updated the minimum dependency version.
The Rust API guidelines advise that all public types implement common
stdtraits for interoperability, of whichfmt::Debugis one.This PR adds
fmt::Debugimplementations for all public types inregexandregex-syntaxcrates. This PR also adds#![warn(missing_debug_implementations)]tolib.rsin each crate.This PR also opportunistically implements
Cloneon iterators that wrapslice::Iter.Context: I'm building an implementation of Ruby
Regexpon top of theregexcrate, and having all public types implementfmt::Debugwill allow me to#[derive(Debug)]on types that wrap e.g. iterators in this crate.