Updating Toolkit To Start Using Cargo Fmt #524

thatzopoulos · 2022-09-06T20:49:28Z

Part 1 of updating toolkit to use cargo fmt.
Once this is merged, we will need to store the merge commit from this PR in a file in the repo so that we can set up git blame to ignore this commit:

Create file .git-blame-ignore-revs:

# Example Commit
864343aaa7d7e72d8e336d8bb8778d4f49846af9

You can use git config to set git to always use the ignoreRevsFile in a project with the following command:

git config blame.ignoreRevsFile .git-blame-ignore-revs

You could also just pass it in as a flag to git blame but then you have to remember to always include the flag.

git blame --ignore-revs-file .git-blame-ignore-revs

thatzopoulos

I went through all of the changes and made a comment on any change that was not just a formatting change to make reviewing a bit easier.

thatzopoulos · 2022-09-07T19:07:33Z

crates/asap/src/lib.rs

@@ -250,9 +264,9 @@ impl<'a> Metrics<'a> {
    }

    fn diffs(&self) -> Vec<f64> {
-        let mut diff = vec!(0.0; (self.len - 1) as usize);
+        let mut diff = vec![0.0; (self.len - 1) as usize];


Clippy update, macros can be invoked using [], (), or {} so this is not functionally any different

thatzopoulos · 2022-09-07T19:16:10Z

crates/encodings/src/lib.rs

@@ -270,7 +277,7 @@ pub mod prefix_varint {
        let tag_byte = value[0];
        if tag_byte & 1 == 1 {
            let value = (tag_byte >> 1) as u64;
-            return (value, 1)
+            return (value, 1);


Unsure if this is clippy or fmt but its functionally the same

crates/flat_serialize/flat_serialize/src/lib.rs

thatzopoulos · 2022-09-07T19:19:50Z

crates/flat_serialize/flat_serialize/src/lib.rs

            Slice::Slice(s) => {
                *self = Slice::Owned(s.to_vec());
-            },
+            }


Note: When using braces in match statements, rustfmt will remove commas. Not a clippy change.

crates/hyperloglogplusplus/src/dense.rs

thatzopoulos · 2022-09-07T19:30:43Z

crates/t-digest/src/lib.rs

@@ -962,7 +962,7 @@ mod tests {
            .chain(batch2.iter())
            .chain(batch3.iter())
            .chain(batch4.iter())
-            .map(|x| *x)
+            .copied()


Clippy change: .copied() creates an iterator which copies all of its elements. This is a cleaner way of doing .map(|x| *x) before .collect()

crates/udd-sketch/src/lib.rs

tools/sql-doctester/src/runner.rs

tools/testrunner/src/main.rs

thatzopoulos · 2022-09-07T20:58:21Z

tools/testrunner/src/main.rs

-    let mut root_client = Client::connect(&*root_connection_config, NoTls)
-        .expect("could not connect to postgres");
+    let mut root_client =
+        Client::connect(root_connection_config, NoTls).expect("could not connect to postgres");


This change seems functionally the same

528: Clippy Suggestions Across Workspaces r=thatzopoulos a=thatzopoulos This PR contains Clippy changes and should be merged before the Cargo Fmt PR: #524 Co-authored-by: Thomas Hatzopoulos <[email protected]>

crates/asap/src/lib.rs

rtwalker

To go along with a few minor comments I had, I'd like to very timidly propose max_width = 120. There's several places where I feel like we get bitten by line length (of course this is a symptom of using formatting tools and it will never be perfect).

Approving because I don't think anything I've seen is worth holding up the PR over

crates/asap/src/lib.rs

crates/counter-agg/src/lib.rs

rtwalker · 2022-09-09T18:31:53Z

extension/src/aggregate_builder_tests.rs

-                "SELECT toolkit_experimental.anything(val) \
+            let output = client
+                .select(
+                    "SELECT toolkit_experimental.anything(val) \


I don't know if it's worth doing (in this PR or at all) but this is one of SEVERAL places where rustfmt makes the SQL statement inside the string have a peculiar alignment.

I think the answer is to use raw string literals and align manually. We have some of those already, as well as some use of raw that still doesn't have clean alignment.

I think we should isolate rustfmt changes to one commit, so if Thomas does feel like doing anything about this right now, let's do it before or after this.

But I think it can just wait.

Along these lines, I suggest NOT squashing this, but keeping the three commits you have now.

rtwalker

Ok I've at least "glanced" at everything now. Seems OK to me!

rtwalker · 2022-09-09T21:15:19Z

extension/src/stats_agg.rs

-                (None, None) => Some(StatsSummary2D::from_internal(InternalStatsSummary2D::new()).into()), // return an empty one from the trans function because otherwise it breaks in the window context
+                (None, None) => {
+                    Some(StatsSummary2D::from_internal(InternalStatsSummary2D::new()).into())
+                } // return an empty one from the trans function because otherwise it breaks in the window context


nit: could turn this into

... // return an empty one from the trans function because otherwise it breaks in the window context Some(StatsSummary2D::from_internal(InternalStatsSummary2D::new()).into()) }

if it's not too much trouble

Added to reviewer comments commit.

epgts

My eyes are bleeding now, but let's do it!

epgts · 2022-09-09T21:13:21Z

extension/src/aggregate_builder_tests.rs

-                "SELECT toolkit_experimental.anything(val) \
+            let output = client
+                .select(
+                    "SELECT toolkit_experimental.anything(val) \


I think the answer is to use raw string literals and align manually. We have some of those already, as well as some use of raw that still doesn't have clean alignment.

I think we should isolate rustfmt changes to one commit, so if Thomas does feel like doing anything about this right now, let's do it before or after this.

But I think it can just wait.

epgts · 2022-09-09T21:15:07Z

extension/src/lib.rs

-#![allow(clippy::extra_unused_lifetimes)]  // some disagreement between clippy and the rust compiler about when lifetime are and are not needed
-#![allow(clippy::not_unsafe_ptr_arg_deref)]  // every function calling in_aggregate_context should be unsafe
+#![allow(clippy::modulo_one)]
+// flat_serialize! alignment checks hit this for any single byte field (of which all pg_types! have two by default)


All these comments now apply to the wrong line.

Added to reviewer comments commit.

thatzopoulos · 2022-09-12T16:58:26Z

bors r+

bors · 2022-09-12T17:10:53Z

Build succeeded:

532: Adding git-blame-ignore-revs file r=thatzopoulos a=thatzopoulos Part 2 of updating toolkit to use `cargo fmt` by uploading `.git-blame-ignore-revs` with the `cargo fmt` commits in the file. Part 1 can be found here: #524 You can use git config to set git to always use the ignoreRevsFile in a project with the following command: ``` git config blame.ignoreRevsFile .git-blame-ignore-revs ``` You could also just pass it in as a flag to git blame but then you have to remember to always include the flag. ``` git blame --ignore-revs-file .git-blame-ignore-revs ``` Co-authored-by: Thomas Hatzopoulos <[email protected]>

thatzopoulos mentioned this pull request Sep 6, 2022

Updating Toolkit To Start Using Cargo Fmt #521

Closed

thatzopoulos requested review from WireBaron, epgts and rtwalker September 6, 2022 20:49

thatzopoulos marked this pull request as ready for review September 6, 2022 20:51

thatzopoulos force-pushed the th/cargo-fmt-all-workspaces branch 2 times, most recently from 7f238c5 to 9f1e33a Compare September 7, 2022 16:26

thatzopoulos commented Sep 7, 2022

View reviewed changes

thatzopoulos mentioned this pull request Sep 8, 2022

Clippy Suggestions Across Workspaces #528

Merged

thatzopoulos force-pushed the th/cargo-fmt-all-workspaces branch from dd7da97 to f1b77a9 Compare September 8, 2022 20:50

Running cargo fmt across all workspaces

8b50127

thatzopoulos force-pushed the th/cargo-fmt-all-workspaces branch from 3385020 to 8b50127 Compare September 8, 2022 20:53

thatzopoulos requested review from syvb and jerryxwu September 9, 2022 18:03

syvb reviewed Sep 9, 2022

View reviewed changes

crates/asap/src/lib.rs Show resolved Hide resolved

rtwalker approved these changes Sep 9, 2022

View reviewed changes

added rustfmt::skip to overly long vec

73b8cd3

rtwalker reviewed Sep 9, 2022

View reviewed changes

epgts approved these changes Sep 9, 2022

View reviewed changes

reviewer comments

11edf5b

thatzopoulos force-pushed the th/cargo-fmt-all-workspaces branch from b89888b to 11edf5b Compare September 12, 2022 16:06

bors bot merged commit b743334 into main Sep 12, 2022

bors bot deleted the th/cargo-fmt-all-workspaces branch September 12, 2022 17:10

thatzopoulos mentioned this pull request Sep 12, 2022

Adding git-blame-ignore-revs file #532

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updating Toolkit To Start Using Cargo Fmt #524

Updating Toolkit To Start Using Cargo Fmt #524

thatzopoulos commented Sep 6, 2022

thatzopoulos left a comment

thatzopoulos Sep 7, 2022

thatzopoulos Sep 7, 2022

thatzopoulos Sep 7, 2022

thatzopoulos Sep 7, 2022

thatzopoulos Sep 7, 2022

rtwalker left a comment •

edited

Loading

rtwalker Sep 9, 2022

epgts Sep 9, 2022

epgts Sep 9, 2022

rtwalker left a comment

rtwalker Sep 9, 2022

thatzopoulos Sep 12, 2022

epgts left a comment

epgts Sep 9, 2022

epgts Sep 9, 2022

thatzopoulos Sep 12, 2022

thatzopoulos commented Sep 12, 2022

bors bot commented Sep 12, 2022

Updating Toolkit To Start Using Cargo Fmt #524

Updating Toolkit To Start Using Cargo Fmt #524

Conversation

thatzopoulos commented Sep 6, 2022

thatzopoulos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rtwalker left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rtwalker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

epgts left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thatzopoulos commented Sep 12, 2022

bors bot commented Sep 12, 2022

rtwalker left a comment •

edited

Loading