Include code format tools #49

kevinlind · 2021-05-24T23:08:38Z

Description

Includes code format tools to build system.

Adds Spotless configuration to Gradle scripts
Configures Spotless with Prettier in edgeidentity module (Java files)
Configures Spotless with ktlint in app module (Kotlin files)
Adds config/formatter/adobe.header.txt used by Spotless to apply copyright header to all Java and Kotlin files
Add Makefile targets to apply and check formatting by running Spotless gradle commands.
Add .githook folder with pre-commit hook to apply formatting on each commit. Include init Makefile target to set git's core.hooksPath to this folder.
Update README with formatting instructions.

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

I have signed the Adobe Open Source CLA.
My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
I have added tests to cover my changes.
All new and existing tests passed.

.circleci/config.yml

.githook/pre-commit

kevinlind · 2021-05-24T23:14:07Z

code/.editorconfig

+ij_visual_guides = 120
+ij_wrap_on_typing = false
+
+[*.java]


I've only included the configurations for Java and Kotlin as those were the only two languages I've configured. I manually removed the configs for other languages when exporting the file from Android Studio.

Are these mostly custom or default settings? Can we keep only the custom ones for easier maintenance?

Any updates on this one?

kevinlind · 2021-05-24T23:15:24Z

code/app/build.gradle

+spotless {
+    kotlin {
+        target "src/*/java/**/*.kt"
+        ktlint('0.41.0')


I decided to use ktlint here to format the Kotlin test app. There were minimal changes to the app files after running the formatting.

kevinlind · 2021-05-24T23:16:52Z

Makefile

+format:
+	(./code/gradlew -p code/$(EXTENSION-LIBRARY-FOLDER-NAME) spotlessApply)
+	(./code/gradlew -p code/$(TEST-APP-FOLDER-NAME) spotlessApply)
+
+format-check:
+	(./code/gradlew -p code/$(EXTENSION-LIBRARY-FOLDER-NAME) spotlessCheck)


I set the make targets to apply formatting to both the source code and test app, but only to check the formatting on the source code. I can add a check to the test app code as well, but I was thinking we wouldn't want to fail a build if the test app source wasn't formatted correctly. Thoughts?

I think it doesn't hurt to add the check for the test app if we anyway run the hook/format cmd on both.

kevinlind · 2021-05-24T23:20:23Z

code/edgeidentity/build.gradle


 jacoco {
    toolVersion = rootProject.ext.jacocoVersion
 }

+preBuild.dependsOn spotlessApply


This will apply the formatting when the project is built. I feel this could be convenient, but as there is the pre-commit hook, if this ends up adding too much time to every build we can remove it.

codecov · 2021-05-25T00:33:34Z

Codecov Report

Merging #49 (14859b2) into dev (0076ec8) will increase coverage by 0.38%.
The diff coverage is 69.10%.

@@            Coverage Diff             @@
##              dev      #49      +/-   ##
==========================================
+ Coverage   79.67%   80.05%   +0.38%     
==========================================
  Files          18       18              
  Lines         777      817      +40     
  Branches      115      124       +9     
==========================================
+ Hits          619      654      +35     
- Misses        114      117       +3     
- Partials       44       46       +2

Flag	Coverage Δ
functional-tests	`59.73% <49.79%> (+1.17%)`	⬆️
unit-tests	`78.45% <68.67%> (+0.47%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...eting/mobile/edge/identity/AuthenticatedState.java	`100.00% <ø> (ø)`
...keting/mobile/edge/identity/IdentityConstants.java	`0.00% <0.00%> (ø)`
...e/marketing/mobile/edge/identity/IdentityItem.java	`60.87% <40.00%> (+0.87%)`	⬆️
...obe/marketing/mobile/edge/identity/EventUtils.java	`53.57% <42.86%> (+7.74%)`	⬆️
...keting/mobile/edge/identity/IdentityExtension.java	`72.79% <54.00%> (+0.07%)`	⬆️
...om/adobe/marketing/mobile/edge/identity/Utils.java	`70.49% <57.14%> (ø)`
...eting/mobile/edge/identity/IdentityProperties.java	`90.16% <60.00%> (-2.82%)`	⬇️
...adobe/marketing/mobile/edge/identity/Identity.java	`69.39% <66.10%> (+0.50%)`	⬆️
...g/mobile/edge/identity/IdentityStorageService.java	`92.00% <90.00%> (ø)`
.../marketing/mobile/edge/identity/IdentityState.java	`92.75% <91.67%> (+0.11%)`	⬆️
... and 8 more

emdobrin

@kevinlind I think the changes look great overall, as I was going through the code I felt it much easier to read and more organized 👍

emdobrin · 2021-05-27T18:23:35Z

Makefile

+format:
+	(./code/gradlew -p code/$(EXTENSION-LIBRARY-FOLDER-NAME) spotlessApply)
+	(./code/gradlew -p code/$(TEST-APP-FOLDER-NAME) spotlessApply)
+
+format-check:
+	(./code/gradlew -p code/$(EXTENSION-LIBRARY-FOLDER-NAME) spotlessCheck)


I think it doesn't hurt to add the check for the test app if we anyway run the hook/format cmd on both.

emdobrin · 2021-05-27T18:26:04Z

code/.editorconfig

+ij_visual_guides = 120
+ij_wrap_on_typing = false
+
+[*.java]


Are these mostly custom or default settings? Can we keep only the custom ones for easier maintenance?

emdobrin · 2021-05-27T18:29:49Z

code/edgeidentity/src/androidTest/java/com/adobe/marketing/mobile/MonitorExtension.java

+		)
+			.build();


this one looks kind of odd for builders but looks good in the Mokito usage for example in testBootupIfReady_whenIdentityDirectRegistered_onFirstBoot_whenIdentityDirectECIDNull_generatesNew

I don't think there is much I can do here as it's how Prettier is conforming to the Google Java Code Style. The issue is really that the builder takes parameters to the constructor, so there are multiple rules being applied. The rule to close the parenthesis puts it in line with the start of the statement, however chained method calls are indented one indentation size. As you pointed out, chained calls without constructor parameters are lined up correctly.

code/edgeidentity/src/androidTest/java/com/adobe/marketing/mobile/TestHelper.java

code/edgeidentity/src/androidTest/java/com/adobe/marketing/mobile/TestPersistenceHelper.java

emdobrin · 2021-05-27T18:55:16Z

code/edgeidentity/src/main/java/com/adobe/marketing/mobile/edge/identity/IdentityExtension.java

+			IdentityConstants.EventType.GENERIC_IDENTITY,
+			IdentityConstants.EventSource.REQUEST_RESET,
+			ListenerIdentityRequestReset.class,
+			listenerErrorCallback


👍 I actually like this a lot better

code/edgeidentity/src/test/java/com/adobe/marketing/mobile/edge/identity/IdentityMapTests.java

emdobrin · 2021-06-03T20:19:13Z

code/edgeidentity/src/test/java/com/adobe/marketing/mobile/edge/identity/IdentityMapTests.java

+			"        ],\n" +
+			"        \"USERID\": [\n" +
+			"          {\n" +
+			"            \"id\":someUserID,\n" +


this should have been the string"someUserID" 🤔

That isn't what it was originally, \"id\":" + "someUserID" + ",\n". If it was supposed to quoted, then the original string should be \"id\":" + "\"someUserID\"" + ",\n".

emdobrin · 2021-06-03T20:19:53Z

code/.editorconfig

+ij_visual_guides = 120
+ij_wrap_on_typing = false
+
+[*.java]


Any updates on this one?

emdobrin · 2021-06-04T21:41:01Z

We decided not to include the .editorconfig file for now

kevinlind added 12 commits May 23, 2021 23:43

Add Spotless format configuration, Makefile targets, and CI step

be5d077

Apply ktlint format to test app files

f231f2d

Disable formatting for IdentityItem.toString

9768573

Apply formatting to Java source

6b65067

Configure Spotless to use spaces instead of tabs

3eea227

Apply format to use spaces instead of tabs

a054530

Apply IDE format which doesn't conflict with spotless/prettier

72afad0

Add .editorconfig file

2c9f67b

Only keep EditorConfig settings for Java and Kotlin.

ae75205

Add pre-commit hook to format code

467b037

Add init target to Makefile to set git hooksPath

7089b2d

Add code format information to README

b50b65d

kevinlind requested review from emdobrin and nporter-adbe May 24, 2021 23:08

kevinlind commented May 24, 2021

View reviewed changes

Include Node.js in CircleCI Android image

d503012

kevinlind added 4 commits May 24, 2021 17:48

Add separate job to validate code

be67729

Configure Spotless and EditorConfig to use tabs, not spaces

0440375

Apply format to use tabs instead of spaces

36c872e

Rename .githook folder to .githooks

58d59fe

emdobrin reviewed May 27, 2021

View reviewed changes

kevinlind added 5 commits June 1, 2021 12:06

Include test app in format-check Makefile target

3326360

Fix formatting of test fail message in TestHelper

3985287

fix formatting of long string

1edc0e6

Initialize known thread list in TestHelper

bd2f486

Add required Node.js version for Prettier in README

b1d0cc7

emdobrin reviewed Jun 3, 2021

View reviewed changes

kevinlind added 2 commits June 3, 2021 16:12

Update .editorconfig to fix the import order

85b3508

Remove .editorconfig file

14859b2

emdobrin approved these changes Jun 4, 2021

View reviewed changes

emdobrin merged commit 3c9f7c6 into adobe:dev Jun 4, 2021

kevinlind deleted the format-spotless branch June 4, 2021 22:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Include code format tools #49

Include code format tools #49

kevinlind commented May 24, 2021 •

edited by emdobrin

Loading

kevinlind May 24, 2021

emdobrin May 27, 2021

emdobrin Jun 3, 2021

kevinlind May 24, 2021

kevinlind May 24, 2021

emdobrin May 27, 2021

kevinlind May 24, 2021

codecov bot commented May 25, 2021 •

edited

Loading

emdobrin left a comment •

edited

Loading

emdobrin May 27, 2021

emdobrin May 27, 2021

emdobrin May 27, 2021

kevinlind Jun 1, 2021

emdobrin May 27, 2021

emdobrin Jun 3, 2021

kevinlind Jun 4, 2021

emdobrin Jun 3, 2021

emdobrin commented Jun 4, 2021

Include code format tools #49

Include code format tools #49

Conversation

kevinlind commented May 24, 2021 • edited by emdobrin Loading

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Checklist:

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

codecov bot commented May 25, 2021 • edited Loading

Codecov Report

emdobrin left a comment • 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

emdobrin commented Jun 4, 2021

kevinlind commented May 24, 2021 •

edited by emdobrin

Loading

codecov bot commented May 25, 2021 •

edited

Loading

emdobrin left a comment •

edited

Loading