Skip to content

Commit 6d64810

Browse files
authored
fix(console): sort attributes alphabetically by name (#210)
Currently, the attributes for async ops are not sorted before displaying them, so the same attribute names may occur in different orders. This is "not great". This branch fixes this by sorting the `Attribute`s prior to formatting them (in `Attribute::make_formatted`), the same way we do in `Field::make_formatted`. I changed the custom comparison closure into an `Ord` impl for `Field` so it could also be used in an `Ord` impl for `Attribute`. As you can see, it works: ![image](https://user-images.githubusercontent.com/2796466/146440281-5cefca2a-75bf-470d-82e5-cdbccc967ed3.png) Fixes #208
1 parent 7d16ead commit 6d64810

File tree

1 file changed

+62
-36
lines changed

1 file changed

+62
-36
lines changed

console/src/state/mod.rs

Lines changed: 62 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ use crate::{
77
use console_api as proto;
88
use std::{
99
cell::RefCell,
10+
cmp::Ordering,
1011
collections::HashMap,
1112
convert::{TryFrom, TryInto},
1213
fmt,
@@ -51,13 +52,13 @@ pub(crate) struct Metadata {
5152
//TODO: add more metadata as needed
5253
}
5354

54-
#[derive(Debug)]
55+
#[derive(Debug, Eq, PartialEq)]
5556
pub(crate) struct Field {
5657
pub(crate) name: InternedStr,
5758
pub(crate) value: FieldValue,
5859
}
5960

60-
#[derive(Debug)]
61+
#[derive(Debug, Eq, PartialEq, Ord, PartialOrd)]
6162
pub(crate) enum FieldValue {
6263
Bool(bool),
6364
Str(String),
@@ -72,7 +73,7 @@ enum Temporality {
7273
Paused,
7374
}
7475

75-
#[derive(Debug)]
76+
#[derive(Debug, Eq, PartialEq)]
7677
pub(crate) struct Attribute {
7778
field: Field,
7879
unit: Option<String>,
@@ -333,31 +334,11 @@ impl Field {
333334
}
334335

335336
fn make_formatted(styles: &view::Styles, fields: &mut Vec<Field>) -> Vec<Vec<Span<'static>>> {
336-
use std::cmp::Ordering;
337-
338337
let key_style = styles.fg(Color::LightBlue).add_modifier(Modifier::BOLD);
339338
let delim_style = styles.fg(Color::LightBlue).add_modifier(Modifier::DIM);
340339
let val_style = styles.fg(Color::Yellow);
341340

342-
fields.sort_unstable_by(|left, right| {
343-
if &*left.name == Field::NAME {
344-
return Ordering::Less;
345-
}
346-
347-
if &*right.name == Field::NAME {
348-
return Ordering::Greater;
349-
}
350-
351-
if &*left.name == Field::SPAWN_LOCATION {
352-
return Ordering::Greater;
353-
}
354-
355-
if &*right.name == Field::SPAWN_LOCATION {
356-
return Ordering::Less;
357-
}
358-
359-
left.name.cmp(&right.name)
360-
});
341+
fields.sort_unstable();
361342

362343
let mut formatted = Vec::with_capacity(fields.len());
363344
let mut fields = fields.iter();
@@ -380,6 +361,29 @@ impl Field {
380361
}
381362
}
382363

364+
impl Ord for Field {
365+
fn cmp(&self, other: &Self) -> Ordering {
366+
match (&*self.name, &*other.name) {
367+
// the `NAME` field should always come first
368+
(Field::NAME, Field::NAME) => Ordering::Equal,
369+
(Field::NAME, _) => Ordering::Less,
370+
(_, Field::NAME) => Ordering::Greater,
371+
372+
// the `SPAWN_LOCATION` field should always come last (it's long)
373+
(Field::SPAWN_LOCATION, Field::SPAWN_LOCATION) => Ordering::Equal,
374+
(Field::SPAWN_LOCATION, _) => Ordering::Greater,
375+
(_, Field::SPAWN_LOCATION) => Ordering::Less,
376+
(this, that) => this.cmp(that),
377+
}
378+
}
379+
}
380+
381+
impl PartialOrd for Field {
382+
fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
383+
Some(self.cmp(other))
384+
}
385+
}
386+
383387
// === impl FieldValue ===
384388

385389
impl fmt::Display for FieldValue {
@@ -417,6 +421,38 @@ impl FieldValue {
417421
}
418422
}
419423

424+
impl From<proto::field::Value> for FieldValue {
425+
fn from(pb: proto::field::Value) -> Self {
426+
match pb {
427+
proto::field::Value::BoolVal(v) => Self::Bool(v),
428+
proto::field::Value::StrVal(v) => Self::Str(v),
429+
proto::field::Value::I64Val(v) => Self::I64(v),
430+
proto::field::Value::U64Val(v) => Self::U64(v),
431+
proto::field::Value::DebugVal(v) => Self::Debug(v),
432+
}
433+
}
434+
}
435+
436+
impl Ord for Attribute {
437+
fn cmp(&self, other: &Self) -> Ordering {
438+
self.field
439+
.cmp(&other.field)
440+
// TODO(eliza): *maybe* this should compare so that larger units are
441+
// greater than smaller units (e.g. `ms` > `us`), rather than
442+
// alphabetically?
443+
// but, meh...
444+
.then_with(|| self.unit.cmp(&other.unit))
445+
}
446+
}
447+
448+
impl PartialOrd for Attribute {
449+
fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
450+
Some(self.cmp(other))
451+
}
452+
}
453+
454+
// === impl Attribute ===
455+
420456
impl Attribute {
421457
fn make_formatted(
422458
styles: &view::Styles,
@@ -427,6 +463,8 @@ impl Attribute {
427463
let val_style = styles.fg(Color::Yellow);
428464
let unit_style = styles.fg(Color::LightBlue);
429465

466+
attributes.sort_unstable();
467+
430468
let mut formatted = Vec::with_capacity(attributes.len());
431469
let attributes = attributes.iter();
432470
for attr in attributes {
@@ -446,18 +484,6 @@ impl Attribute {
446484
}
447485
}
448486

449-
impl From<proto::field::Value> for FieldValue {
450-
fn from(pb: proto::field::Value) -> Self {
451-
match pb {
452-
proto::field::Value::BoolVal(v) => Self::Bool(v),
453-
proto::field::Value::StrVal(v) => Self::Str(v),
454-
proto::field::Value::I64Val(v) => Self::I64(v),
455-
proto::field::Value::U64Val(v) => Self::U64(v),
456-
proto::field::Value::DebugVal(v) => Self::Debug(v),
457-
}
458-
}
459-
}
460-
461487
fn truncate_registry_path(s: String) -> String {
462488
use once_cell::sync::OnceCell;
463489
use regex::Regex;

0 commit comments

Comments
 (0)