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

Missing fmt::Debug impls for public structs and enums #735

Merged
merged 2 commits into from
Dec 29, 2020
Merged

Missing fmt::Debug impls for public structs and enums #735

merged 2 commits into from
Dec 29, 2020

Conversation

lopopolo
Copy link
Contributor

The Rust API guidelines advise that all public types implement common std traits for interoperability, of which fmt::Debug is one.

This PR adds fmt::Debug implementations for all public types in regex and regex-syntax crates. This PR also adds #![warn(missing_debug_implementations)] to lib.rs in each crate.

This PR also opportunistically implements Clone on iterators that wrap slice::Iter.

Context: I'm building an implementation of Ruby Regexp on top of the regex crate, and having all public types implement fmt::Debug will allow me to #[derive(Debug)] on types that wrap e.g. iterators in this crate.

Copy link
Member

@BurntSushi BurntSushi left a comment

Choose a reason for hiding this comment

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

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/dfa.rs Outdated Show resolved Hide resolved
src/re_trait.rs Outdated
.field("last_match", &self.last_match)
.finish()
}
}
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I also made the same change to CaptureMatches.

src/exec.rs Outdated Show resolved Hide resolved
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)]
Copy link
Member

Choose a reason for hiding this comment

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

Is this Debug impl necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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)]
Copy link
Member

Choose a reason for hiding this comment

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

Is this Debug impl necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Compiler is public via the internal module which "exists to support suspicious activity".

I'll remove the debug impl and allow the lint.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

@BurntSushi BurntSushi left a comment

Choose a reason for hiding this comment

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

Whoops, I meant to "request changes."

@lopopolo
Copy link
Contributor Author

lopopolo commented Dec 29, 2020

@BurntSushi I've made the requested changes. I squashed these into the commit for the regex crate.

For types that are only public via the hidden internal module, I've added an #[allow(missing_debug_implementations)] and avoid implementing Debug.

@lopopolo
Copy link
Contributor Author

rebase incoming.

@lopopolo
Copy link
Contributor Author

LGTM, but like the other PR

For reference, the other PR is #734.

Copy link
Member

@BurntSushi BurntSushi left a comment

Choose a reason for hiding this comment

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

OK, LGTM, thanks!

@BurntSushi BurntSushi merged commit ee94996 into rust-lang:master Dec 29, 2020
@lopopolo
Copy link
Contributor Author

Whoops looks like I accidentally didn't split out the iterator traits bits from regex-syntax properly when splitting apart the two PRs.

Thanks for merging.

Would it be possible to get these two PRs packaged into a release soon?

@BurntSushi
Copy link
Member

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.

@lopopolo
Copy link
Contributor Author

lopopolo commented Jan 6, 2021

hi @BurntSushi following up with a friendly ping for a release.

@BurntSushi
Copy link
Member

@lopopolo OK, regex 1.4.3 is on crates.io.

@lopopolo
Copy link
Contributor Author

lopopolo commented Jan 8, 2021

Thank you!

This was referenced Mar 12, 2021
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