Update AST builder ExprNode & ExprList to accept borrowed values #967

panarch · 2022-10-23T10:43:40Z

Update ExprNode & ExprList to accept both String and &str to build ExprNode.
Update ExprList to accept both owned vector and borrowed slice.

e.g. ExprList

        // Vec<Into<ExprNode>>
        let actual: ExprList = vec!["id", "name"].into();
        let expected = "id, name";
        test(actual, expected);

        // Vec<ExprNode>
        let actual: ExprList = vec![col("id"), col("name")].into();
        let expected = "id, name";
        test(actual, expected);

        // &Vec<ExprNode>
        let actual = vec!["id", "name"];
        let actual: ExprList = (&actual).into();
        let expected = "id, name";
        test(actual, expected);

        // &[Into<ExprNode>]
        let actual = vec!["rate / 10", "col1"];
        let actual: ExprList = actual.as_slice().into();
        let expected = "rate / 10, col1";
        test(actual, expected);

        // &[ExprNode]
        let actual = vec![col("id"), col("name")];
        let actual: ExprList = actual.as_slice().into();
        let expected = "id, name";
        test(actual, expected);

coveralls · 2022-10-23T10:54:33Z

Pull Request Test Coverage Report for Build 3306764259

123 of 123 (100.0%) changed or added relevant lines in 3 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.08%) to 97.892%

Totals
Change from base Build 3303918887:	0.08%
Covered Lines:	38227
Relevant Lines:	39050

💛 - Coveralls

ever0de

Looks good 👍 👍 👍 👍 👍

ever0de · 2022-10-23T10:57:26Z

core/src/ast_builder/expr_list.rs

+                let exprs = exprs.into_owned();
+
+                exprs
+                    .into_iter()
+                    .map(TryInto::try_into)
+                    .collect::<Result<Vec<_>>>()


https://github.com/rust-lang/rust-clippy/blob/967f172e256802f9bf0ad5718d9c761da18132a9/clippy_lints/src/methods/unnecessary_to_owned.rs#L198-L239

It gives an unnecessary_to_owned clippy error, and clippy tells you to fix it like this:

T.iter().cloned()

But it seems rather unnecessary referencing.
The behavior of into_owned is only to strip the enum when Cow::Owned, whereas Iterator::cloned calls Clone::clone.

So we separated it like this.

…esql#967) Update ExprNode & ExprList to accept both String and &str to build ExprNode. Update ExprList to accept both owned vector and borrowed slice.

Update AST builder ExprNode & ExprList to accept borrowed values

f766f98

panarch added the improvement Improvements for existing features label Oct 23, 2022

panarch requested review from ever0de, 24seconds and devgony October 23, 2022 10:43

panarch self-assigned this Oct 23, 2022

ever0de approved these changes Oct 23, 2022

View reviewed changes

panarch merged commit 80ebf7a into main Oct 23, 2022

panarch deleted the exprnode-to-get-borrowed-values branch October 23, 2022 11:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update AST builder ExprNode & ExprList to accept borrowed values #967

Update AST builder ExprNode & ExprList to accept borrowed values #967

panarch commented Oct 23, 2022

coveralls commented Oct 23, 2022

ever0de left a comment

ever0de Oct 23, 2022

Update AST builder ExprNode & ExprList to accept borrowed values #967

Update AST builder ExprNode & ExprList to accept borrowed values #967

Conversation

panarch commented Oct 23, 2022

coveralls commented Oct 23, 2022

Pull Request Test Coverage Report for Build 3306764259

💛 - Coveralls

ever0de left a comment

Choose a reason for hiding this comment

ever0de Oct 23, 2022

Choose a reason for hiding this comment