Clippy lints + clippy CI #341

joshfried-aws · 2023-03-10T14:20:15Z

Issue #, if available:
#208

Description of changes:
As part of our goal to improve code quality, we are addressing all clippy lints in our project + adding a clippy check to our CI

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

akshayrane · 2023-03-28T23:48:27Z

guard/src/commands/files.rs

@@ -14,35 +13,14 @@ pub(crate) fn read_file_content(file: File) -> Result<String, std::io::Error> {
    Ok(file_content)
 }

-pub(crate) fn get_files<F>(file: &str, sort: F) -> Result<Vec<PathBuf>, Error>


Is this function not used anymore?

akshayrane · 2023-03-28T23:50:17Z

guard/src/commands/helper.rs

 pub fn validate_and_return_json(
    data: ValidateInput,
    rules: ValidateInput,
    verbose: bool,
 ) -> Result<String> {
-    let input_data = match serde_json::from_str::<serde_json::Value>(&data.content) {
+    let input_data = match serde_json::from_str::<serde_json::Value>(data.content) {


Does changing reference to value not have any effect here?

No it doesnt...Since the content field of ValidateInput is already a &str the reference infront of &data.content is immediately being dereferenced by the compiler

akshayrane · 2023-03-28T23:56:29Z

guard/src/commands/validate/cfn.rs

-            })
+            };
+
+            Ok(())


In the older code, aren't we returning a writer as a result? In the newer one we are not?

Ok(... serde_yaml::to_writer())

The function signature is unchanged. Those 3 methods inside the if statement all return a unit type. This is exactly the reason why its best practice to not to do something like

Ok(some_fn() -> Result<()>)

it makes it seem like we're returning something more than a unit type

Kotlin has 'Unit' too

akshayrane · 2023-03-28T23:58:49Z

guard/src/commands/validate/cfn.rs

@@ -465,7 +432,7 @@ fn handle_resource_aggr<'record, 'value: 'record>(
    root: &'value Node<'_>,
    name: String,
    by_resources: &mut HashMap<String, LocalResourceAggr<'record, 'value>>,
-    value: &Vec<Rc<crate::commands::validate::common::Node<'record, 'value>>>,
+    value: &[Rc<crate::commands::validate::common::Node<'record, 'value>>],


Are we changing a data type here?

Technically yes, but it doesnt require any change other than function signature. https://rust-lang.github.io/rust-clippy/master/index.html#ptr_arg

akshayrane · 2023-03-28T23:59:35Z

guard/src/commands/validate/cfn_reporter.rs

@@ -181,7 +172,7 @@ impl super::common::GenericReporter for SingleLineReporter {
        by_resource_name: HashMap<String, Vec<NameInfo<'_>>>,
        passed: HashSet<String>,
        skipped: HashSet<String>,
-        longest_rule_len: usize,
+        _longest_rule_len: usize,


I am assuming Clippy adds _ as prefix to variables that aren't used? Can you confirm?

akshayrane · 2023-03-29T00:24:12Z

guard/src/rules/path_value.rs

+    //     copy.push('/');
+    //     copy.push_str(part);
+    //     Path(copy, loc)
+    // }


Why is this chunk commented out?

Compiler was complaining about unused code, and i commented it out and ran the test suite + did manual testing to make sure it wasnt actually being used...Forgot to delete it. Good catch

akshayrane · 2023-03-29T00:28:01Z

guard/src/rules/functions/strings.rs

@@ -1,194 +0,0 @@
-use crate::rules::path_value::{Path, PathAwareValue};


Why are these removed?

Wasnt being used anywhere

guard/src/rules/functions/collections.rs

guard/src/rules/eval_context_tests.rs

akshayrane · 2023-03-29T00:36:32Z

guard/src/rules/eval/operators.rs

-        QueryResult::UnResolved(ur) => Err(err(ur.clone())),
-    }
-}
+// pub(super) fn resolved<'value, E, R>(


Why is this chunk commented out?

same as the previous commented out chunk

…images

* Added 2 runners to integration tests for rules registry * Fixed indent * Added explicit shell name * Moved shell to job parameters * Added powershell commands for windows * Removed test branch * Updated README.md (aws-cloudformation#352) * Updated README for Guard 3.0 * Update README.md Co-authored-by: Ben Bridts <ben.bridts@gmail.com> --------- Co-authored-by: Ben Bridts <ben.bridts@gmail.com> --------- Co-authored-by: Akshay Rane <raneaks@amazon.com> Co-authored-by: razcloud <34892703+razcloud@users.noreply.github.com> Co-authored-by: Ben Bridts <ben.bridts@gmail.com>

…ssary imports

andrewjeminchoi · 2023-06-12T14:42:12Z

.github/workflows/pr.yml

@@ -2,9 +2,9 @@ name: Rust

 on:
  push:
-    branches: [ main, development, rogue_one]
+    branches: [ main, development, rogue_one ]


andrewjeminchoi · 2023-06-12T14:45:15Z

guard/src/rules/path_value.rs

-
-    pub(crate) fn extend_string(&self, part: &String) -> Path {
-        self.extend_str(part.as_str())
+    //


Please consider deletion

joshfried-aws · 2023-06-13T17:22:54Z

rejecting this in favour of #371

joshfried-aws requested review from razcloud, wkexiang, Brianwithay21, reza-aws, Neel-Renavikar and akshayrane March 10, 2023 19:37

joshfried-aws force-pushed the main branch from 73a826d to 358367a Compare March 23, 2023 17:24

joshfried-aws changed the base branch from main to development March 23, 2023 20:52

joshfried-aws force-pushed the main branch from 358367a to 71774cb Compare March 28, 2023 20:59

akshayrane reviewed Mar 29, 2023

View reviewed changes

joshfried-aws force-pushed the main branch from f67ef87 to cd4ef93 Compare June 8, 2023 19:31

joshfried-aws changed the base branch from development to rogue_one June 8, 2023 19:31

joshfried-aws force-pushed the main branch from cd4ef93 to 7f29eff Compare June 8, 2023 19:50

Akshay Rane and others added 8 commits June 9, 2023 10:13

Updated README.md to have disclaimer for package managers and latest …

52cd7f3

…images

addressing all clippy lints in project + adding clippy check to ci

7c6b0c5

more clippy lints

a36e5fa

clippy lints for test code

4764ce1

readding clippy check after mistake in rebase

2c94b10

adding back functions crate

709310a

temporarily ignoring dead code for functional module + removing unece…

f08879a

…ssary imports

joshfried-aws force-pushed the main branch from 7f29eff to f08879a Compare June 9, 2023 14:14

andrewjeminchoi reviewed Jun 12, 2023

View reviewed changes

joshfried-aws closed this Jun 13, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clippy lints + clippy CI #341

Clippy lints + clippy CI #341

joshfried-aws commented Mar 10, 2023

akshayrane Mar 28, 2023

joshfried-aws Mar 29, 2023

akshayrane Mar 28, 2023

joshfried-aws Mar 29, 2023

akshayrane Mar 28, 2023

joshfried-aws Mar 29, 2023 •

edited

brayalad Apr 6, 2023

akshayrane Mar 28, 2023

joshfried-aws Mar 29, 2023

akshayrane Mar 28, 2023

joshfried-aws Mar 29, 2023

akshayrane Mar 29, 2023

joshfried-aws Mar 29, 2023

akshayrane Mar 29, 2023

joshfried-aws Mar 29, 2023

akshayrane Mar 29, 2023

joshfried-aws Mar 29, 2023

andrewjeminchoi Jun 12, 2023

andrewjeminchoi Jun 12, 2023

joshfried-aws commented Jun 13, 2023

-                          })
+                          };
+                          Ok(())

		@@ -1,194 +0,0 @@
		use crate::rules::path_value::{Path, PathAwareValue};

Clippy lints + clippy CI #341

Clippy lints + clippy CI #341

Conversation

joshfried-aws commented Mar 10, 2023

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

joshfried-aws Mar 29, 2023 • edited

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshfried-aws commented Jun 13, 2023

joshfried-aws Mar 29, 2023 •

edited