Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a lint rule for ensuring usage of internal version of collectAsState. #8497

Merged
merged 2 commits into from
Jun 10, 2024

Conversation

samer-stripe
Copy link
Collaborator

@samer-stripe samer-stripe commented May 21, 2024

Summary

Add a lint rule for ensuring usage of of internal version of collectAsState.

Motivation

Ensures we continue using of internal version of collectAsState in the future.

Testing

  • Added tests
  • Modified tests
  • Manually verified

@samer-stripe samer-stripe requested review from a team as code owners May 21, 2024 05:45
@samer-stripe samer-stripe requested review from awush-stripe and removed request for a team May 21, 2024 05:45
@samer-stripe samer-stripe force-pushed the samer/safely-observe-state-flows branch from e4f1639 to a442567 Compare May 21, 2024 05:45
@samer-stripe samer-stripe removed request for a team and awush-stripe May 21, 2024 05:46
@samer-stripe samer-stripe marked this pull request as draft May 21, 2024 05:46
@samer-stripe samer-stripe changed the title Samer/add lint check for collect as state Add lint check for using custom version of collectAsState. May 21, 2024
@samer-stripe samer-stripe changed the title Add lint check for using custom version of collectAsState. Add lint check for using custom version of collectAsState. May 21, 2024
Copy link
Contributor

github-actions bot commented May 21, 2024

Diffuse output:

OLD: paymentsheet-example-release-master.apk (signature: V1, V2)
NEW: paymentsheet-example-release-pr.apk (signature: V1, V2)

          │            compressed            │           uncompressed           
          ├───────────┬───────────┬──────────┼───────────┬───────────┬──────────
 APK      │ old       │ new       │ diff     │ old       │ new       │ diff     
──────────┼───────────┼───────────┼──────────┼───────────┼───────────┼──────────
      dex │     4 MiB │     4 MiB │   +955 B │   8.7 MiB │   8.7 MiB │ +1,016 B 
     arsc │   2.2 MiB │   2.2 MiB │      0 B │   2.2 MiB │   2.2 MiB │      0 B 
 manifest │   5.1 KiB │   5.1 KiB │      0 B │  25.8 KiB │  25.8 KiB │      0 B 
      res │ 910.1 KiB │ 910.1 KiB │      0 B │   1.4 MiB │   1.4 MiB │      0 B 
   native │   2.6 MiB │   2.6 MiB │      0 B │     6 MiB │     6 MiB │      0 B 
    asset │   2.9 MiB │   2.9 MiB │ +1.4 KiB │   2.9 MiB │   2.9 MiB │ +1.4 KiB 
    other │ 194.3 KiB │ 194.3 KiB │     +8 B │ 424.7 KiB │ 424.7 KiB │      0 B 
──────────┼───────────┼───────────┼──────────┼───────────┼───────────┼──────────
    total │  12.8 MiB │  12.8 MiB │ +2.3 KiB │  21.7 MiB │  21.7 MiB │ +2.3 KiB 

 DEX     │ old   │ new   │ diff              
─────────┼───────┼───────┼───────────────────
   files │     1 │     1 │   0               
 strings │ 43406 │ 43371 │ -35 (+48 -83)     
   types │ 14908 │ 14911 │  +3 (+47 -44)     
 classes │ 12598 │ 12601 │  +3 (+40 -37)     
 methods │ 61656 │ 61665 │  +9 (+2352 -2343) 
  fields │ 40643 │ 40651 │  +8 (+764 -756)   

 ARSC    │ old  │ new  │ diff 
─────────┼──────┼──────┼──────
 configs │  242 │  242 │  0   
 entries │ 6033 │ 6033 │  0
APK
     compressed      │     uncompressed     │                                           
──────────┬──────────┼───────────┬──────────┤                                           
 size     │ diff     │ size      │ diff     │ path                                      
──────────┼──────────┼───────────┼──────────┼───────────────────────────────────────────
  7.3 KiB │ +1.3 KiB │   7.2 KiB │ +1.3 KiB │ ∆ assets/dexopt/baseline.prof             
    4 MiB │   +955 B │   8.7 MiB │ +1,016 B │ ∆ classes.dex                             
    880 B │    +12 B │     748 B │    +12 B │ ∆ assets/dexopt/baseline.profm            
 49.5 KiB │     +4 B │ 116.8 KiB │      0 B │ ∆ META-INF/MANIFEST.MF                    
 52.8 KiB │     +3 B │ 116.9 KiB │      0 B │ ∆ META-INF/CERT.SF                        
    272 B │     +1 B │     120 B │      0 B │ ∆ META-INF/version-control-info.textproto 
──────────┼──────────┼───────────┼──────────┼───────────────────────────────────────────
  4.1 MiB │ +2.3 KiB │   8.9 MiB │ +2.3 KiB │ (total)
DEX
STRINGS:

   old   │ new   │ diff          
  ───────┼───────┼───────────────
   43406 │ 43371 │ -35 (+48 -83) 
  
  + LF6/S1;
  + LH9/b;
  + LH9/c;
  + LH9/d;
  + LH9/e;
  + LH9/f;
  + LH9/g;
  + LH9/h;
  + LH9/i;
  + LH9/j;
  + LH9/k;
  + LH9/l;
  + LH9/m;
  + LH9/n;
  + LH9/o;
  + LH9/p;
  + LH9/q;
  + LH9/r;
  + LH9/s;
  + LH9/t;
  + LH9/u;
  + LH9/v;
  + LH9/w;
  + LH9/x;
  + LH9/y;
  + LK9/c;
  + LK9/d;
  + LP9/c;
  + LP9/d;
  + LS9/c;
  + LS9/d;
  + LS9/e;
  + LS9/f;
  + LT9/a;
  + LT9/b;
  + LV9/a;
  + LV9/b;
  + Ln8/k;
  + Ln8/l;
  + Lp0/s1;
  + [LH9/t;
  + [LM6/j;
  + [LR9/e;
  + [LV9/a;
  + [Lp0/D0;
  + [Lp0/s1;
  + [Lp0/z0;
  + ~~R8{backend:dex,compilation-mode:release,has-checksums:false,min-api:21,pg-map-id:c03ac03,r8-mode:full,version:8.3.37}
  
  - A2
  - B2
  - C2
  - D2
  - E2
  - F2
  - G2
  - H2
  - I2
  - J2
  - K2
  - L2
  - LE9/a;
  - LE9/b;
  - LF9/c;
  - LF9/d;
  - LG9/e;
  - LG9/f;
  - LG9/g;
  - LG9/h;
  - LG9/i;
  - LG9/j;
  - LG9/k;
  - LG9/l;
  - LG9/m;
  - LG9/n;
  - LG9/o;
  - LG9/p;
  - LG9/q;
  - LG9/r;
  - LG9/s;
  - LG9/t;
  - LG9/u;
  - LG9/v;
  - LG9/w;
  - LG9/x;
  - LG9/y;
  - LI9/b;
  - LI9/c;
  - LJ9/d;
  - LO9/c;
  - LO9/d;
  - LQ6/d;
  - LQ9/c;
  - LQ9/d;
  - LQ9/e;
  - LR9/f;
  - LU9/a;
  - LU9/b;
  - M2
  - N2
  - O2
  - P2
  - Q2
  - R2
  - S2
  - T2
  - U2
  - V2
  - W2
  - [LG9/t;
  - [LQ9/e;
  - [LU9/a;
  - [Le3/a;
  - [Lp0/C0;
  - [Lp0/r1;
  - [Lp0/y0;
  - i2
  - j2
  - k2
  - m2
  - n2
  - o2
  - q2
  - r2
  - s2
  - t2
  - u2
  - w2
  - x2
  - y2
  - z2
  - ~~R8{backend:dex,compilation-mode:release,has-checksums:false,min-api:21,pg-map-id:00f25d8,r8-mode:full,version:8.3.37}
  

TYPES:

   old   │ new   │ diff         
  ───────┼───────┼──────────────
   14908 │ 14911 │ +3 (+47 -44) 
  
  + LF6/S1;
  + LH9/b;
  + LH9/c;
  + LH9/d;
  + LH9/e;
  + LH9/f;
  + LH9/g;
  + LH9/h;
  + LH9/i;
  + LH9/j;
  + LH9/k;
  + LH9/l;
  + LH9/m;
  + LH9/n;
  + LH9/o;
  + LH9/p;
  + LH9/q;
  + LH9/r;
  + LH9/s;
  + LH9/t;
  + LH9/u;
  + LH9/v;
  + LH9/w;
  + LH9/x;
  + LH9/y;
  + LK9/c;
  + LK9/d;
  + LP9/c;
  + LP9/d;
  + LS9/c;
  + LS9/d;
  + LS9/e;
  + LS9/f;
  + LT9/a;
  + LT9/b;
  + LV9/a;
  + LV9/b;
  + Ln8/k;
  + Ln8/l;
  + Lp0/s1;
  + [LH9/t;
  + [LM6/j;
  + [LR9/e;
  + [LV9/a;
  + [Lp0/D0;
  + [Lp0/s1;
  + [Lp0/z0;
  
  - LE9/a;
  - LE9/b;
  - LF9/c;
  - LF9/d;
  - LG9/e;
  - LG9/f;
  - LG9/g;
  - LG9/h;
  - LG9/i;
  - LG9/j;
  - LG9/k;
  - LG9/l;
  - LG9/m;
  - LG9/n;
  - LG9/o;
  - LG9/p;
  - LG9/q;
  - LG9/r;
  - LG9/s;
  - LG9/t;
  - LG9/u;
  - LG9/v;
  - LG9/w;
  - LG9/x;
  - LG9/y;
  - LI9/b;
  - LI9/c;
  - LJ9/d;
  - LO9/c;
  - LO9/d;
  - LQ6/d;
  - LQ9/c;
  - LQ9/d;
  - LQ9/e;
  - LR9/f;
  - LU9/a;
  - LU9/b;
  - [LG9/t;
  - [LQ9/e;
  - [LU9/a;
  - [Le3/a;
  - [Lp0/C0;
  - [Lp0/r1;
  - [Lp0/y0;
  

METHODS:

   old   │ new   │ diff             
  ───────┼───────┼──────────────────
   61656 │ 61665 │ +9 (+2352 -2343) 
  
  + A3.e b(f, float) → float
  + A4.c f() → String
  + A4.c w0() → Map
  + A4.d f() → String
  + A4.d w0() → Map
  + A4.e f() → String
  + A4.e w0() → Map
  + A4.f f() → String
  + A4.f w0() → Map
  + A4.g f() → String
  + A4.g w0() → Map
  + A5.j <init>(P1, L0, M, e)
  + A5.n <init>(e, String, String, e, e, P1)
  + A7.M <init>(d0, c0, N, c, H, K, i0, h, e, f)
  + A7.a0 t0(String, Throwable)
  + A7.a0 y0()
  + A7.b0 t0(String, Throwable)
  + A7.b0 y0()
  + A9.a X1() → String
  + A9.b <init>(int, int, b, e, d, String)
  + A9.c <init>(int, int, a, String)
  + C.i A(Object)
  + C.i B(k) → b
  + C.i C() → Handler
  + C.i D(k) → d
  + C.i E(Object[]) → int
  + C.i F(int, int) → int
  + C.i G()
  + C.i H(int, int, int) → int
  + C.i I(JSONObject) → g
  + C.i J(JSONObject) → S1
  + C.i K(int, int) → int
  + C.i L(k) → m
  + C.i M(Object[], q, String, a, k, int) → Object
  + C.i N()
  + C.i O()
  + C.i P()
  + C.i Q(int, k) → String
  + C.i R(int, Object[], k) → String
  + C.i S(long) → long
  + C.i T(int, int, int, int) → boolean
  + C.i U(Context, e, q)
  + C.i a(q, boolean, k, boolean, k, int)
  + C.i b(long, g, e, k, int)
  + C.i c(int, int) → long
  + C.i d(a, q, E, e, k, int, int)
  + C.i e(Object, int, B, e, k, int)
  + C.i f(E, s, g0, k, int)
  + C.i g(f, k, int)
  + C.i h(long, boolean, k, boolean, q, e, k, int)
  + C.i i(n, Object, int, Object, k, int)
  + C.i j(int, h) → int
  + C.i k(D) → boolean
  + C.i l(Object[], int, Object, Object) → Object[]
  + C.i m(int, Object[]) → Object[]
  + C.i n(int, Ob
...✂

Base automatically changed from samer/safely-observe-state-flows to master May 22, 2024 14:05
@samer-stripe samer-stripe force-pushed the samer/add-lint-check-for-collect-as-state branch from b2b65f9 to f21a606 Compare May 22, 2024 14:23
@samer-stripe samer-stripe marked this pull request as ready for review May 22, 2024 14:23
@samer-stripe samer-stripe changed the title Add lint check for using custom version of collectAsState. Add a lint rule for ensuring usage of collectAsStateSafely. May 22, 2024
@samer-stripe
Copy link
Collaborator Author

I'm wondering if we want to rename collectAsStateSafely to collectAsState as well and just let the lint rule ensure we use the correct one.

@samer-stripe samer-stripe marked this pull request as draft May 22, 2024 14:27
@jaynewstrom-stripe
Copy link
Collaborator

I don't have a strong preference on naming, if the lint rule works.

@samer-stripe samer-stripe marked this pull request as ready for review May 22, 2024 14:40
@samer-stripe
Copy link
Collaborator Author

Will rename in a separate PR

@jaynewstrom-stripe
Copy link
Collaborator

This must still not be triggered. There are collectAsState references in financial connections and identity.

@samer-stripe samer-stripe force-pushed the samer/add-lint-check-for-collect-as-state branch 2 times, most recently from aee5319 to b6b4d8c Compare June 4, 2024 23:34
@samer-stripe
Copy link
Collaborator Author

@jaynewstrom-stripe Working as expected now, I can add a separate commit that replaces all the usages once this code looks good.

@jaynewstrom-stripe
Copy link
Collaborator

Sounds good to me.

@samer-stripe samer-stripe force-pushed the samer/add-lint-check-for-collect-as-state branch from b6b4d8c to 10539bf Compare June 5, 2024 16:03
@samer-stripe samer-stripe requested review from a team as code owners June 5, 2024 16:03
@samer-stripe samer-stripe force-pushed the samer/add-lint-check-for-collect-as-state branch from 10539bf to a76f98c Compare June 5, 2024 16:16
@@ -1,3 +1,5 @@
@file:Suppress("ComposeCollectAsStateUsageIssue")
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only one I didn't replace in production code. Financial connections has a custom collectAsState here that has additional logic.

@samer-stripe
Copy link
Collaborator Author

This replaces all the usages in the SDK outside of those found in our example modules (paymentsheet-example, and example).

ccen-stripe
ccen-stripe previously approved these changes Jun 5, 2024
Copy link
Collaborator

@ccen-stripe ccen-stripe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm for Identity

@samer-stripe samer-stripe changed the title Add a lint rule for ensuring usage of collectAsStateSafely. Add a lint rule for ensuring usage of internal version of collectAsState. Jun 6, 2024
@samer-stripe samer-stripe force-pushed the samer/add-lint-check-for-collect-as-state branch from a76f98c to 58bb4c7 Compare June 10, 2024 15:01
@samer-stripe samer-stripe merged commit bb35fd4 into master Jun 10, 2024
16 checks passed
@samer-stripe samer-stripe deleted the samer/add-lint-check-for-collect-as-state branch June 10, 2024 22:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants