Closed Bug 1488006 Opened Last year Closed Last year

mozprofile: Fix clippy lint warnings and errors

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

(12 files, 2 obsolete files)

67.13 KB, patch
jgraham
: review+
Details | Diff | Splinter Review
3.60 KB, patch
jgraham
: review+
Details | Diff | Splinter Review
1.97 KB, patch
jgraham
: review+
Details | Diff | Splinter Review
2.03 KB, patch
jgraham
: review+
Details | Diff | Splinter Review
2.93 KB, patch
jgraham
: review+
Details | Diff | Splinter Review
1.17 KB, patch
jgraham
: review+
Details | Diff | Splinter Review
2.50 KB, patch
jgraham
: review+
Details | Diff | Splinter Review
1.07 KB, patch
jgraham
: review+
Details | Diff | Splinter Review
3.38 KB, patch
jgraham
: review+
Details | Diff | Splinter Review
1.44 KB, patch
jgraham
: review+
Details | Diff | Splinter Review
897 bytes, patch
jgraham
: review+
Details | Diff | Splinter Review
47 bytes, text/x-phabricator-request
Details | Review
Per title.
Assignee: nobody → ato
Status: NEW → ASSIGNED
No functional changes, apart from running all the code through
rustfmt prior to fixing clippy lint warnings.
Attachment #9005831 - Flags: review?(james)
Drops all the unnecessary lifetime definitions in mozprofile.
Attachment #9005833 - Flags: review?(james)
Where string literals contain only ASCII characters, it is considered
better style to define byte strings using b"foo" rather than calling
"foo".as_bytes().
Attachment #9005834 - Flags: review?(james)
io::Write::write() is not guaranteed to process the entire buffer.
It returns how many bytes were processed, which may be smaller than the
given buffer's length.  mozprofile does not need to deal with partial
writes, so all the calls to ::write() may be replaced with ::write_all().
Attachment #9005835 - Flags: review?(james)
It is considered more idiomatic Rust to loop over references to
containers rather than calling the iteration protocol's x.iter()
or x.into_iter() explicitly.
Attachment #9005836 - Flags: review?(james)
PrefReaderError::new already expects a &str.
Attachment #9005837 - Flags: review?(james)
Where we have single-character strings we can convert them to chars
for efficiency.
Attachment #9005838 - Flags: review?(james)
It is considered more idiomatic to dereference the match expression
than to peek at each variant through a reference.

This silences a clippy lint warning.
Attachment #9005839 - Flags: review?(james)
There is a functional call after ok_or() which is called and
evaluated before escape_char is converted due to RHS evaluation.
We can avoid this by employing a closure with ok_or_else().
Attachment #9005841 - Flags: review?(james)
Users might expect that Position can be used with the Default trait
if we define a ::new() function on it.  It might be clearer to rename
this function ::start() so that it is explicit what the position is.

This silences the following clippy warning:
https://rust-lang-nursery.github.io/rust-clippy/v0.0.212/index.html#new_without_default_derive
Attachment #9005842 - Flags: review?(james)
Attachment #9005831 - Flags: review?(james) → review+
Attachment #9005832 - Flags: review?(james) → review+
Attachment #9005833 - Flags: review?(james) → review+
Attachment #9005834 - Flags: review?(james) → review+
Attachment #9005835 - Flags: review?(james) → review+
Attachment #9005836 - Flags: review?(james) → review+
Attachment #9005837 - Flags: review?(james) → review+
Attachment #9005838 - Flags: review?(james) → review+
Attachment #9005839 - Flags: review?(james) → review+
Comment on attachment 9005840 [details] [diff] [review]
mozprofile: avoid manual implementation of assign operator

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

<<= seems way more obscure to me; I'd prefer to ignore that lint.
Attachment #9005841 - Flags: review?(james) → review+
Comment on attachment 9005842 [details] [diff] [review]
mozprofile: rename Position::new() to ::start()

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

This seems like a needless change to the public API for a very hypothetical problem.
Attachment #9005842 - Flags: review?(james) → review-
(In reply to James Graham [:jgraham] from comment #14)
> Comment on attachment 9005842 [details] [diff] [review]
> mozprofile: rename Position::new() to ::start()
> 
> Review of attachment 9005842 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This seems like a needless change to the public API for a very hypothetical
> problem.

Then we need to implement the Default trait instead to make clippy
happy.  I thought it was semantically better to rename it, but I
can’t submit a patch for other way.
The user might expect to be able to use Default as the type can be
constructed without arguments.
Attachment #9006244 - Flags: review?(james)
Attachment #9005842 - Attachment is obsolete: true
Comment on attachment 9006244 [details] [diff] [review]
webdriver: derive Default for prefreader::Position.

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

I would also have been very happy to just change the configuration to silence the warning in this case.
Attachment #9006244 - Flags: review?(james) → review+
Comment on attachment 9005840 [details] [diff] [review]
mozprofile: avoid manual implementation of assign operator

> Then we need to implement the Default trait instead to make clippy
> happy.  I thought it was semantically better to rename it, but I
> can’t submit a patch for other way.

I actually can’t figure out how to do this, so I’m going to drop
that patch.
Attachment #9005840 - Attachment is obsolete: true
Attachment #9005840 - Flags: review?(james)
(In reply to James Graham [:jgraham] from comment #17)
> Comment on attachment 9006244 [details] [diff] [review]
> webdriver: derive Default for prefreader::Position.
> 
> Review of attachment 9006244 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I would also have been very happy to just change the configuration to
> silence the warning in this case.

Yeah, but I couldn’t figure out how to (-:
Pushed by ato@sny.no:
https://hg.mozilla.org/integration/mozilla-inbound/rev/03fde1788da9
mozprofile: format code; r=jgraham
https://hg.mozilla.org/integration/mozilla-inbound/rev/a680beaf94c4
mozprofile: avoid redundant field names in struct init; r=jgraham
https://hg.mozilla.org/integration/mozilla-inbound/rev/86af1d4f044e
mozprofile: drop needless lifetimes; r=jgraham
https://hg.mozilla.org/integration/mozilla-inbound/rev/a03963a35bcd
mozprofile: prefer byte string literals to as_bytes() where possible; r=jgraham
https://hg.mozilla.org/integration/mozilla-inbound/rev/7b9df7da7d39
mozprofile: replace call to io::Write::write() with ::write_all(); r=jgraham
https://hg.mozilla.org/integration/mozilla-inbound/rev/7f018c99822e
mozprofile: loop over references to container; r=jgraham
https://hg.mozilla.org/integration/mozilla-inbound/rev/c8373045be47
mozprofile: avoid identical conversions; r=jgraham
https://hg.mozilla.org/integration/mozilla-inbound/rev/5b38507b76cb
mozprofile: turn single-character strings into chars; r=jgraham
https://hg.mozilla.org/integration/mozilla-inbound/rev/1e63eff2c381
mozprofile: deref token to avoid & prefix; r=jgraham
https://hg.mozilla.org/integration/mozilla-inbound/rev/4136b6f97a9a
mozprofile: avoid incidental functional call; r=jgraham
https://hg.mozilla.org/integration/mozilla-inbound/rev/41b45139750f
mozprofile: derive Default for prefreader::Position. r=jgraham
Target Milestone: mozilla64 → mozilla63
Target Milestone: mozilla63 → mozilla64
Pushed by james@hoppipolla.co.uk:
https://hg.mozilla.org/integration/autoland/rev/69f4d4c7a175
Use assignment operator instead of manual implementation r=jgraham
You need to log in before you can comment on or make changes to this bug.