Closed Bug 1488007 Opened 2 years ago Closed 2 years ago

mozversion: Fix clippy lints

Categories

(Testing :: Mozbase Rust, enhancement)

enhancement
Not set

Tracking

(firefox64 fixed)

RESOLVED FIXED
mozilla64
Tracking Status
firefox64 --- fixed

People

(Reporter: ato, Assigned: ato)

Details

Attachments

(2 files)

Per title.
Assignee: nobody → ato
Status: NEW → ASSIGNED
Attached patch mozversion: lintSplinter Review
Address clippy lint warnings in mozversion.
Attachment #9005843 - Flags: review?(james)
No functional changes, except applying rustfmt.
Attachment #9005844 - Flags: review?(james)
Comment on attachment 9005844 [details] [diff] [review]
mozversion: format

Review of attachment 9005844 [details] [diff] [review]:
-----------------------------------------------------------------

::: testing/mozbase/rust/mozversion/src/lib.rs
@@ +118,5 @@
>          if let Some(captures) = version_re.captures(version_string) {
> +            match captures
> +                .name("major")
> +                .and_then(|x| u64::from_str(x.as_str()).ok())
> +            {

I hadn't considered the possibility that someone might invent a style where the { sometimes goes on a new line and sometimes not. That's impressively ugly.
Attachment #9005844 - Flags: review?(james) → review+
Attachment #9005843 - Flags: review?(james) → review+
(In reply to James Graham [:jgraham] from comment #3)

> ::: testing/mozbase/rust/mozversion/src/lib.rs
> @@ +118,5 @@
> >          if let Some(captures) = version_re.captures(version_string) {
> > +            match captures
> > +                .name("major")
> > +                .and_then(|x| u64::from_str(x.as_str()).ok())
> > +            {
> 
> I hadn't considered the possibility that someone might invent a style where
> the { sometimes goes on a new line and sometimes not. That's impressively
> ugly.

I see what you mean, but I think it happens to not give the impression
the new block is a continuation of the chained expression.

Compare for eaxmple:

> match captures
>     .name("major")
>     .and_then(|x| u64::from_str(x.as_str()).ok())
> {
>     new_block();
> }

To:

> match captures
>     .name("major")
>     .and_then(|x| u64::from_str(x.as_str()).ok()) {
>     new_block();
> }

However I think maybe we can avoid this altogether by splitting
this expression up.  I will do that in the patch I push.
Actually I won’t because this code is a bit complicated.
https://hg.mozilla.org/mozilla-central/rev/52af3a20610d
https://hg.mozilla.org/mozilla-central/rev/66a735968247
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Target Milestone: mozilla64 → mozilla63
Target Milestone: mozilla63 → mozilla64
You need to log in before you can comment on or make changes to this bug.