Remove lint warnings #337

hannesa2 · 2022-07-11T05:04:16Z

I followed the suggestion from lint

codecov-commenter · 2022-07-11T05:42:32Z

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 85.71429% with 1 line in your changes missing coverage. Please review.

Project coverage is 87.07%. Comparing base (9b18315) to head (c972329).
Report is 250 commits behind head on master.

Files with missing lines	Patch %	Lines
...java/org/matomo/sdk/dispatcher/EventDiskCache.java	0.00%	1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@             Coverage Diff              @@
##             master     #337      +/-   ##
============================================
+ Coverage     86.78%   87.07%   +0.28%     
- Complexity      371      372       +1     
============================================
  Files            33       32       -1     
  Lines          1415     1408       -7     
  Branches        165      164       -1     
============================================
- Hits           1228     1226       -2     
+ Misses          117      114       -3     
+ Partials         70       68       -2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

d4rken

Some of the changes affect the public API that devs might be using (see comment), the rest looks good, thanks for the cleanup 👍

tracker/src/main/java/org/matomo/sdk/Matomo.java

d4rken · 2022-07-11T09:10:31Z

tracker/src/main/java/org/matomo/sdk/TrackMe.java

@@ -30,9 +30,8 @@ public TrackMe(TrackMe trackMe) {
    /**
     * Adds TrackMe to this TrackMe, overriding values if necessary.
     */
-    public TrackMe putAll(@NonNull TrackMe trackMe) {
+    public void putAll(@NonNull TrackMe trackMe) {


This changes the public API.
It returns itself so devs can chain calls.
e.g. TrackMe().putAll(...).trySet(...)

Fine, but the sample is anyhow to outdated and too simple #338

Do you mean extending the sample app so the linter no longer marks this as unused?

I don't think the sample app has to include all and every API call the SDK has, I'm not opposed to anyone making the sample app better either 😄)

We just have to be a bit careful with refactoring.
The linter doesn't know it's an SDK and code is used outside the project 😁

Sure, I as a customer will only use stuff I find in sample app. It's just a note.

The linter doesn't know it's an SDK and code is used outside the project

yes, maybe annotate it with "ignore warning" is a way to go

Till now, I ignored my wish to kotlize it.
Kotlin has some wonderful mechanism for a sdk, e.g internal

d4rken · 2022-07-11T09:10:55Z

tracker/src/main/java/org/matomo/sdk/TrackMe.java

     */
-    public synchronized TrackMe trySet(@NonNull QueryParams key, int value) {
-        return trySet(key, String.valueOf(value));
+    public synchronized void trySet(@NonNull QueryParams key, int value) {


Changes public API, see above.

d4rken · 2022-07-11T09:11:07Z

tracker/src/main/java/org/matomo/sdk/TrackMe.java

     */
-    public synchronized TrackMe trySet(@NonNull QueryParams key, float value) {
-        return trySet(key, String.valueOf(value));
+    public synchronized void trySet(@NonNull QueryParams key, float value) {


Changes public API, see above.

d4rken · 2022-07-11T09:11:11Z

tracker/src/main/java/org/matomo/sdk/TrackMe.java

    }

-    public synchronized TrackMe trySet(@NonNull QueryParams key, long value) {
-        return trySet(key, String.valueOf(value));
+    public synchronized void trySet(@NonNull QueryParams key, long value) {


Changes public API, see above.

d4rken · 2022-07-11T09:11:16Z

tracker/src/main/java/org/matomo/sdk/TrackMe.java

     */
-    public synchronized TrackMe trySet(@NonNull QueryParams key, String value) {
+    public synchronized void trySet(@NonNull QueryParams key, String value) {


Changes public API, see above.

d4rken · 2022-07-11T09:12:00Z

tracker/src/main/java/org/matomo/sdk/TrackerBuilder.java

@@ -43,9 +43,8 @@ public int getSiteId() {
    /**
     * A unique name for this Tracker. Used to store Tracker settings independent of URL and id changes.
     */
-    public TrackerBuilder setTrackerName(String name) {
+    public void setTrackerName(String name) {


Changes public API (chaining calls on a builder object Builder().doA().doB()

d4rken · 2022-07-11T09:12:13Z

tracker/src/main/java/org/matomo/sdk/TrackerBuilder.java

@@ -58,9 +57,8 @@ public String getTrackerName() {
     *
     * @param domain your-domain.com
     */
-    public TrackerBuilder setApplicationBaseUrl(String domain) {
+    public void setApplicationBaseUrl(String domain) {


Changes public API

d4rken · 2022-07-11T09:12:21Z

tracker/src/main/java/org/matomo/sdk/extra/CustomVariables.java

@@ -70,9 +70,8 @@ public CustomVariables(@Nullable String json) {
        }
    }

-    public CustomVariables putAll(CustomVariables customVariables) {
+    public void putAll(CustomVariables customVariables) {


Changes public API

d4rken · 2022-07-11T09:13:06Z

tracker/src/main/java/org/matomo/sdk/extra/DownloadTracker.java

-        abstract class Custom implements Extra {
-        }
-


I agree, this is kind of superfluos, but it's part of the public API and some devs might have extended this.

Keeping this unused stuff is a kind of collect a lot waste in your base ground.

We can remove it, but then with a major version bump and a little note about migration in the changelog (e.g. don't extend Custom, just implement Extra).

d4rken

Thank you!

d4rken · 2022-07-11T12:17:24Z

Till now, I ignored my wish to kotlize it.
Kotlin has some wonderful mechanism for a sdk, e.g internal

You could do a Kotlin rewrite if you want :).
Would probably looks very nice.
Requires a major version bump and some taking care that the APIs do not totally break for people using this with Java still.

hannesa2 · 2022-07-11T12:23:19Z

Nice, we come closer

hannesa2 · 2022-07-11T14:42:02Z

Would probably looks very nice.
Requires a major version bump and some taking care that the APIs do not totally break for people using this with Java still.

In an ideal world, the api is done by interfaces and anything behind will be done internal. As long as you don't change the interface you can convert without any doubt what you want.

This keeping unused classes because someone could use it, is an afford without benefit.

hannesa2 · 2022-07-11T14:44:32Z

But this interface way isn't done

d4rken · 2022-07-11T15:16:07Z

This keeping unused classes because someone could use it, is an afford without benefit.

Why do you keep coming back to this? 😄
I know it's not great, I probably just forgot about that class. I had no one to review my code.
It's out there now and we shouldn't break the users app "just because it's not clean".
Sorry for not writing the perfect SDK? 🤔

But this interface way isn't done

Not everything needs to be a firebase-level SDK with shovels of abstraction.
This is volunteer work and the SDK worked fine for my use-cases, so at that time: Why make things more complicated if there is no requirement?
A rewrite to Kotlin wasn't on the horizon at that point.

Interfaces can still be introduced now if desired. As said, you can make a v5, with everything new and better if you want to 👍.

hannesa2 · 2022-07-11T18:17:00Z

Sorry, those were just a few thoughts on the Kotlin transformation.
You are right, house holding of our time is also important.

hannesa2 force-pushed the LintWarnings branch from 8c17f05 to ecab9c7 Compare July 11, 2022 05:38

hannesa2 added the enhancement label Jul 11, 2022

d4rken requested changes Jul 11, 2022

View reviewed changes

Remove lint warnings

c972329

hannesa2 force-pushed the LintWarnings branch 2 times, most recently from 8b21ffa to c972329 Compare July 11, 2022 09:27

d4rken approved these changes Jul 11, 2022

View reviewed changes

d4rken merged commit a36429f into master Jul 11, 2022

d4rken deleted the LintWarnings branch July 11, 2022 12:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove lint warnings #337

Remove lint warnings #337

hannesa2 commented Jul 11, 2022

codecov-commenter commented Jul 11, 2022 •

edited

Loading

d4rken left a comment

d4rken Jul 11, 2022

hannesa2 Jul 11, 2022

d4rken Jul 11, 2022

hannesa2 Jul 11, 2022

hannesa2 Jul 11, 2022

d4rken Jul 11, 2022

d4rken Jul 11, 2022

d4rken Jul 11, 2022

d4rken Jul 11, 2022

d4rken Jul 11, 2022

d4rken Jul 11, 2022

d4rken Jul 11, 2022

d4rken Jul 11, 2022

hannesa2 Jul 11, 2022

d4rken Jul 11, 2022

d4rken left a comment

d4rken commented Jul 11, 2022

hannesa2 commented Jul 11, 2022

hannesa2 commented Jul 11, 2022

hannesa2 commented Jul 11, 2022

d4rken commented Jul 11, 2022

hannesa2 commented Jul 11, 2022

Remove lint warnings #337

Remove lint warnings #337

Conversation

hannesa2 commented Jul 11, 2022

codecov-commenter commented Jul 11, 2022 • edited Loading

Codecov Report

d4rken 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

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

d4rken left a comment

Choose a reason for hiding this comment

d4rken commented Jul 11, 2022

hannesa2 commented Jul 11, 2022

hannesa2 commented Jul 11, 2022

hannesa2 commented Jul 11, 2022

d4rken commented Jul 11, 2022

hannesa2 commented Jul 11, 2022

codecov-commenter commented Jul 11, 2022 •

edited

Loading