Clippy warnings fixed & added to CI. #378

ShreyBana · 2025-01-22T12:17:05Z

Fixed all lint warnings. (Some shortcuts have been taken)
Added targets in make to lint & format files, additionally a helper for commiting whilst checking these.

* feat: Add auth via OAUTH2 * feat: Make auth endpoints part of AuthHandler

- Removed nested/recursive make calls in targets. - Made docker conditonal, w/ podman as an alternative tool.

ShubhranshuSanjeev · 2025-01-22T12:48:07Z

makefile

@@ -166,4 +168,26 @@ test: setup frontend superposition
 tailwind:
 	cd crates/frontend && npx tailwindcss -i ./styles/tailwind.css -o ./pkg/style.css --watch

+format:
+	cargo fmt $(FMT_FLAGS)


cd crates/frontend && leptosfmt . && cd ../..
Let's add leptosfmt also

Datron

TIL a.clone() and *a do the same thing

Datron · 2025-01-23T05:50:16Z

crates/context_aware_config/src/api/config/handlers.rs

+    #[cfg(feature = "high-performance-mode")]
+    let scope = scope.service(get_config_fast);
+    Scope::new("")
        .service(get_config)
        .service(get_resolved_config)
        .service(reduce_config)
-        .service(get_config_versions);
-    #[cfg(feature = "high-performance-mode")]
-    let scope = scope.service(get_config_fast);
-    scope
+        .service(get_config_versions)


This won't work, scope.service isn't declared or defined yet. The new() is happening in the next line

Datron · 2025-01-23T05:51:23Z

crates/context_aware_config/src/api/config/handlers.rs

@@ -159,7 +158,7 @@ pub fn generate_config_from_version(
    tenant: &Tenant,
 ) -> superposition::Result<Config> {
    if let Some(val) = version {
-        let val = val.clone();
+        let val = *val;


Opinion, .clone() is more clear on what's happening to val

I just ran cargo clippy --fix, this is what the community prefers ig. But I think there is a way to turn this thing off, can other folks comment on this, we'll take a call accordingly.
@juspay/sdk-backend

@Datron this should not have been a clone at all over here

Datron · 2025-01-23T05:54:34Z

crates/experimentation_platform/src/api/experiments/handlers.rs

-    let exp_context = Exp::<Condition>::try_from(req.context.clone())
-        .map_err(|err| {
-            log::error!("failed to decode condition with error {}", err);
-            bad_argument!(err)
-        })?
-        .into_inner();


Does this work because the type is specified below? What about the error handling?

It was a redundant operation, it was doing try_from from (& into) the same type.

Datron · 2025-01-23T05:55:20Z

crates/experimentation_platform/src/api/experiments/types.rs

            .collect::<Map<_, _>>();

        let toss = value
            .remove("toss")
            .and_then(|toss| toss.as_i64())
            .and_then(|toss| {
-                if -1 <= toss && toss <= 100 {
+                if (-1..=100).contains(&toss) {


This seems like a worse check, earlier comparison was cleaner. This looks a bit overkill

Lmao, the F is this 🤣, let me see if we can turn this off.

Datron · 2025-01-23T05:59:16Z

crates/frontend/src/components/dimension_form.rs

@@ -252,7 +256,7 @@ where
                            on:keypress=move |ev| {
                                let char_code = ev.char_code();
                                if char_code != 0 && char_code != 8 && char_code != 13
-                                    && !(char_code >= 48 && char_code <= 57)
+                                    && !(48..=57).contains(&char_code)


I kind of prefer the older way. Why make this change? Not blocking on this though

Datron · 2025-01-23T06:00:45Z

crates/frontend/src/components/input_components.rs

@@ -46,7 +45,7 @@ pub fn enum_dropdown(
                        disabled=disabled
                        dropdown_width="w-100"
                        dropdown_icon="".to_string()
-                        dropdown_text=(move || { selected_enum.get() })()
+                        dropdown_text=selected_enum.get()


Does this need to be a closure to be reactive?

Closure gets created & is being consumed immediately 🥹

Datron · 2025-01-23T06:02:52Z

crates/frontend/src/logic.rs

@@ -41,7 +41,7 @@ impl Expression {
                .cloned()
                .ok_or("Invalid operands list for context")?;

-            let operand_0 = operands.get(0);
+            let operand_0 = operands.first();


nitpick: can we keep it get(0) for consistency with the below statements?

Datron · 2025-01-23T06:06:29Z

crates/service_utils/src/aws/kms.rs

@@ -16,9 +16,9 @@ pub async fn decrypt(aws_kms_cli: Client, key: &str) -> String {
        .await;
    let key_value: String = String::from_utf8(
        key_value_bytes_result
-            .expect(&format!("Failed to decrypt {key}"))
+            .unwrap_or_else(|_| panic!("Failed to decrypt {key}"))


Is this a clippy suggestion? What reason did it give for this change?

this rationale is pretty correct, often this rationale has been ignored while development but luckily we did not have any side effect in all of our expects and unwrap_ors, so we were lucky

in practice, its always better to have unwrap_or_else at the very least, instead of unwrap_or

Datron · 2025-01-23T06:51:20Z

@ShreyBana there are some issues with leptosfmt, we don't need to add it right now. ~~Also lets not format (both leptosfmt and cargofmt) in the CI, instead lets just check and fail as before.~~

Edit: Saw that you added check flags, ignore the last sentence.

* refactor: workspace ui & form * refactor: moved api types behind feature flag

ShubhranshuSanjeev · 2025-01-23T10:13:47Z

@ShreyBana there are some issues with leptosfmt, we don't need to add it right now. ~~Also lets not format (both leptosfmt and cargofmt) in the CI, instead lets just check and fail as before.~~

Edit: Saw that you added check flags, ignore the last sentence.

We did have issues with leptosfmt in the past, @ShreyBana can you format the frontend code once with leptosfmt and check whether things work as suppose to on your local.
If things work fine, we can keep leptosfmt in the CI.

ci: Added lint check in CI.

ayushjain17 · 2025-01-29T09:33:45Z

crates/context_aware_config/src/api/dimension/types.rs

@@ -15,11 +15,12 @@ pub struct CreateReq {

 #[derive(Debug, Deserialize, AsRef, Deref, DerefMut, Into, Clone)]
 #[serde(try_from = "i32")]
+#[derive(Default)]


can you combine this with the other Derives which is there above

sauraww and others added 30 commits January 9, 2025 12:51

feat: added update api for default config (#310)

9f04e9f

feat: Use Common db model types in frontend (#291)

1e64494

refactor: merge cac and experimentation schemas

cea143a

feat: Add auth via OAUTH2 (#321)

eb2212d

* feat: Add auth via OAUTH2 * feat: Make auth endpoints part of AuthHandler

feat: added schema and crl apis for organisation (#322)

06bbe1f

feat: JsonLogic algebraic representation

5051ce9

fix: JSONSchema generated forms for contexts

a1db40d

feat: added description and comment (#284)

e98473f

refactor: using juspay diesel fork

ce0edf2

refactor: added schema_name dsl to cac queries

d8df7fe

refactor: added schema_name dsl to experimentation queries

3565c40

chore: formatting

e641f31

chore: merge cac and experimentation schemas

8a8ba43

feat: added workspaces table and workspace management APIs

ee82868

feat: workspace-ui

6a5fe0c

feat: update UI to accept org ID

1d26705

feat: Redirect to workspaces after selecting org_id

853e43d

fix: added missing returning DSLs

14c6d80

fix: update Tenant and org in all frontend pages

f0856bf

fix: Redirect to workspaces after selecting org_id in local

d08223c

feat: auth path prefix support and org_user authentication

4093c64

fix: Formatting

0fff32b

fix: read from workspaces table when showing the dropdown

a683b97

fix: org_id in tests

158b39f

fix: add org_id in tests headers

6cb53b8

fix: add org tests

eae34f6

fix: experiment handlers to send org_id

a7ede3e

fix: get types test failure

e1a9795

fix: workspace template SQL and workspace update handler

6debfe4

fix: revert experiment list pagination bug

a075dc4

ShubhranshuSanjeev and others added 6 commits January 20, 2025 20:19

fix: resolved comments

d94c657

fix: Fixed update w/ accurate verbage.

27294be

fix: set toggle length and default monaco strings

7b6a26f

fix: Resolving general pending issues (#375)

5dea34d

refactor: Refactored make & related files. (#339)

b36eb2e

- Removed nested/recursive make calls in targets. - Made docker conditonal, w/ podman as an alternative tool.

fix: mandatory dimensions with workspaces (#370)

37ba8b1

ShreyBana requested a review from a team as a code owner January 22, 2025 12:17

ShreyBana force-pushed the fix+ci/clippy branch from 1aef89d to 1ba9aba Compare January 22, 2025 12:21

ShubhranshuSanjeev requested changes Jan 22, 2025

View reviewed changes

ShreyBana force-pushed the fix+ci/clippy branch 3 times, most recently from 6159466 to 3451045 Compare January 22, 2025 13:22

Datron requested changes Jan 23, 2025

View reviewed changes

refactor: workspace ui & form (#373)

7eb8ee5

* refactor: workspace ui & form * refactor: moved api types behind feature flag

refactor: OrgId, WorkspaceId, SchemaName cleanup and refactor (#379)

48213f1

ShreyBana force-pushed the fix+ci/clippy branch from 3451045 to 58a1cd2 Compare January 23, 2025 11:19

fix: Fixed clippy warnings.

4d08fdf

ci: Added lint check in CI.

ShreyBana force-pushed the fix+ci/clippy branch from 58a1cd2 to 4d08fdf Compare January 23, 2025 12:00

Datron force-pushed the saas branch from 8b12e6b to 3266519 Compare January 23, 2025 12:27

Base automatically changed from saas to main January 23, 2025 13:18

ayushjain17 reviewed Jan 29, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clippy warnings fixed & added to CI. #378

Clippy warnings fixed & added to CI. #378

ShreyBana commented Jan 22, 2025

ShubhranshuSanjeev Jan 22, 2025

Datron left a comment

Datron Jan 23, 2025

Datron Jan 23, 2025

ShreyBana Jan 23, 2025 •

edited

Loading

ayushjain17 Jan 29, 2025

Datron Jan 23, 2025

ShreyBana Jan 23, 2025

Datron Jan 23, 2025

ShreyBana Jan 23, 2025

Datron Jan 23, 2025

Datron Jan 23, 2025

ShreyBana Jan 23, 2025

Datron Jan 23, 2025

ShreyBana Jan 23, 2025

Datron Jan 23, 2025

ShreyBana Jan 23, 2025 •

edited

Loading

ayushjain17 Jan 29, 2025 •

edited

Loading

Datron commented Jan 23, 2025 •

edited

Loading

ShubhranshuSanjeev commented Jan 23, 2025

ayushjain17 Jan 29, 2025

Clippy warnings fixed & added to CI. #378

Are you sure you want to change the base?

Clippy warnings fixed & added to CI. #378

Conversation

ShreyBana commented Jan 22, 2025

Choose a reason for hiding this comment

Datron left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShreyBana Jan 23, 2025 • 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

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

ShreyBana Jan 23, 2025 • edited Loading

Choose a reason for hiding this comment

ayushjain17 Jan 29, 2025 • edited Loading

Choose a reason for hiding this comment

Datron commented Jan 23, 2025 • edited Loading

ShubhranshuSanjeev commented Jan 23, 2025

Choose a reason for hiding this comment

ShreyBana Jan 23, 2025 •

edited

Loading

ShreyBana Jan 23, 2025 •

edited

Loading

ayushjain17 Jan 29, 2025 •

edited

Loading

Datron commented Jan 23, 2025 •

edited

Loading