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

chore: fix clippy errors #375

Merged
merged 1 commit into from
Sep 4, 2022
Merged

chore: fix clippy errors #375

merged 1 commit into from
Sep 4, 2022

Conversation

Noah-Kennedy
Copy link
Contributor

Needed for #374.

@Noah-Kennedy
Copy link
Contributor Author

Oh these issues are with generated files!

@Noah-Kennedy
Copy link
Contributor Author

@hawkw should I have clippy ignore the generated files?

@hawkw
Copy link
Member

hawkw commented Sep 4, 2022

@hawkw should I have clippy ignore the generated files?

yeah, probably...that's annoying. :|

@Noah-Kennedy
Copy link
Contributor Author

Done.

@Noah-Kennedy
Copy link
Contributor Author

Alright, it looks to be passing CI. Is squashing enabled for this repo?

Copy link
Member

@hawkw hawkw left a comment

Choose a reason for hiding this comment

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

i'm fine with merging this as-is in order to fix CI, but if you don't mind, it might be nice to change the blanket allow for the whole console-api crate so that it doesn't include the human-written code in console_api::common.

@@ -1,4 +1,5 @@
#![doc = include_str!("../README.md")]
#![allow(warnings)]
Copy link
Member

Choose a reason for hiding this comment

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

hmm, could we, instead, add the attribute only to the modules that are generated by prost? the common module also contains some handwritten code:

impl From<tracing_core::Level> for metadata::Level {
fn from(level: tracing_core::Level) -> Self {
match level {
tracing_core::Level::ERROR => metadata::Level::Error,
tracing_core::Level::WARN => metadata::Level::Warn,
tracing_core::Level::INFO => metadata::Level::Info,
tracing_core::Level::DEBUG => metadata::Level::Debug,
tracing_core::Level::TRACE => metadata::Level::Trace,
}
}
}
impl From<tracing_core::metadata::Kind> for metadata::Kind {
fn from(kind: tracing_core::metadata::Kind) -> Self {
// /!\ Note that this is intentionally *not* implemented using match.
// The `metadata::Kind` struct in `tracing_core` was written not
// intending to allow exhaustive matches, but accidentally did.
//
// Therefore, we shouldn't be able to write a match against both
// variants without a wildcard arm. However, on versions of
// `tracing_core` where the type was exhaustively matchable, a wildcard
// arm will result in a warning. Thus we must write this rather
// tortured-looking `if` statement to get non-exhaustive matching
// behavior.
if kind == tracing_core::metadata::Kind::SPAN {
metadata::Kind::Span
} else {
metadata::Kind::Event
}
}
}
impl<'a> From<&'a tracing_core::Metadata<'a>> for Metadata {
fn from(meta: &'a tracing_core::Metadata<'a>) -> Self {
let kind = if meta.is_span() {
metadata::Kind::Span
} else {
debug_assert!(meta.is_event());
metadata::Kind::Event
};
let field_names = meta.fields().iter().map(|f| f.name().to_string()).collect();
Metadata {
name: meta.name().to_string(),
target: meta.target().to_string(),
location: Some(meta.into()),
kind: kind as i32,
level: metadata::Level::from(*meta.level()) as i32,
field_names,
..Default::default()
}
}
}
impl<'a> From<&'a tracing_core::Metadata<'a>> for Location {
fn from(meta: &'a tracing_core::Metadata<'a>) -> Self {
Location {
file: meta.file().map(String::from),
module_path: meta.module_path().map(String::from),
line: meta.line(),
column: None, // tracing doesn't support columns yet
}
}
}
impl<'a> From<&'a std::panic::Location<'a>> for Location {
fn from(loc: &'a std::panic::Location<'a>) -> Self {
Location {
file: Some(loc.file().to_string()),
line: Some(loc.line()),
column: Some(loc.column()),
..Default::default()
}
}
}
impl fmt::Display for field::Value {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
match self {
field::Value::BoolVal(v) => fmt::Display::fmt(v, f)?,
field::Value::StrVal(v) => fmt::Display::fmt(v, f)?,
field::Value::U64Val(v) => fmt::Display::fmt(v, f)?,
field::Value::DebugVal(v) => fmt::Display::fmt(v, f)?,
field::Value::I64Val(v) => fmt::Display::fmt(v, f)?,
}
Ok(())
}
}
impl fmt::Display for Field {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
let name_val = (self.name.as_ref(), self.value.as_ref());
if let (Some(field::Name::StrName(name)), Some(val)) = name_val {
write!(f, "{}={}", name, val)?;
}
Ok(())
}
}
impl fmt::Display for Location {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
match (self.module_path.as_ref(), self.file.as_ref()) {
// Module paths take precedence because they're shorter...
(Some(module), _) => f.write_str(module.as_ref())?,
(None, Some(file)) => f.write_str(file.as_ref())?,
// If there's no file or module path, then printing the line and
// column makes no sense...
(None, None) => return f.write_str("<unknown location>"),
};
if let Some(line) = self.line {
write!(f, ":{}", line)?;
// Printing the column only makes sense if there's a line...
if let Some(column) = self.column {
write!(f, ":{}", column)?;
}
}
Ok(())
}
}
// === IDs ===
impl From<&'static tracing_core::Metadata<'static>> for MetaId {
fn from(meta: &'static tracing_core::Metadata) -> Self {
MetaId {
id: meta as *const _ as u64,
}
}
}
impl From<tracing_core::span::Id> for SpanId {
fn from(id: tracing_core::span::Id) -> Self {
SpanId { id: id.into_u64() }
}
}
impl From<SpanId> for tracing_core::span::Id {
fn from(span_id: SpanId) -> Self {
tracing_core::span::Id::from_u64(span_id.id)
}
}
impl From<u64> for SpanId {
fn from(id: u64) -> Self {
SpanId { id }
}
}
impl From<&'static tracing_core::Metadata<'static>> for register_metadata::NewMetadata {
fn from(meta: &'static tracing_core::Metadata) -> Self {
register_metadata::NewMetadata {
id: Some(meta.into()),
metadata: Some(meta.into()),
}
}
}
impl From<i64> for field::Value {
fn from(val: i64) -> Self {
field::Value::I64Val(val)
}
}
impl From<u64> for field::Value {
fn from(val: u64) -> Self {
field::Value::U64Val(val)
}
}
impl From<bool> for field::Value {
fn from(val: bool) -> Self {
field::Value::BoolVal(val)
}
}
impl From<&str> for field::Value {
fn from(val: &str) -> Self {
field::Value::StrVal(val.into())
}
}
impl From<&str> for field::Name {
fn from(val: &str) -> Self {
field::Name::StrName(val.into())
}
}
impl From<&dyn std::fmt::Debug> for field::Value {
fn from(val: &dyn std::fmt::Debug) -> Self {
field::Value::DebugVal(format!("{:?}", val))
}
}
// Clippy warns when a type derives `PartialEq` but has a manual `Hash` impl,
// or vice versa. However, this is unavoidable here, because `prost` generates
// a struct with `#[derive(PartialEq)]`, but we cannot add`#[derive(Hash)]` to the
// generated code.
#[allow(clippy::derive_hash_xor_eq)]
impl Hash for field::Name {
fn hash<H: Hasher>(&self, state: &mut H) {
match self {
field::Name::NameIdx(idx) => idx.hash(state),
field::Name::StrName(s) => s.hash(state),
}
}
}
impl Eq for field::Name {}
// === IDs ===
impl From<u64> for Id {
fn from(id: u64) -> Self {
Id { id }
}
}
impl From<Id> for u64 {
fn from(id: Id) -> Self {
id.id
}
}
impl Copy for Id {}
impl From<tracing_core::span::Id> for Id {
fn from(id: tracing_core::span::Id) -> Self {
Id { id: id.into_u64() }
}
}
and it would be nice if clippy could lint that. we might have to put the generated code in a submodule of common, like this:

// in `common.rs`

mod generated {
    #![allow(warnings)]
    include!("generated/rs.tokio.console.common.rs");
}

pub use self::generated::*;

// ...

for the other modules, we can just put the #[allow(warnings)] attribute on the declaration of those modules.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@Noah-Kennedy
Copy link
Contributor Author

squashed

@Noah-Kennedy
Copy link
Contributor Author

@hawkw I don't have merge permissions for this repo, so you'll have to do it.

@hawkw hawkw merged commit faaf808 into tokio-rs:main Sep 4, 2022
@Noah-Kennedy
Copy link
Contributor Author

Thanks!

@Noah-Kennedy Noah-Kennedy deleted the noah/clippy branch September 4, 2022 20:57
hawkw pushed a commit that referenced this pull request Sep 29, 2023
Needed for #374.

This configures clippy to ignore most of the generated code in
`console-api`.
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