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

Adding linting and linted project #445

Closed
wants to merge 3 commits into from

Conversation

Ben2356
Copy link
Contributor

@Ben2356 Ben2356 commented Oct 26, 2021

Added linting via https://github.com/JLLeitschuh/ktlint-gradle which utilizes KTLint . To run the linter against the codebase, do a rebuild on the project through Build > Rebuild Project then run either ktlintCheck to find any linting issues or run ktlintFormat where it will find issues and if it can fix them automatically it will, otherwise the build fails and you will have to manually fix the issues. You can see a list of the rules that KTLint enforces here: https://ktlint.github.io/ . We have the ability to add more rules as needed or tweak existing ones.

I have ran ktlintFormat on all of the codebase and made manual changes where needed but now the entire codebase should be successfully linted 😃.

I don't have access to make the changes but I think we should probably add a linting github check for PRs to enforce the new style and ensure that future code that is pushed in is consistent.

@codecov-commenter
Copy link

codecov-commenter commented Oct 26, 2021

Codecov Report

Merging #445 (077ef26) into main (b7a3475) will increase coverage by 9.07%.
The diff coverage is 63.11%.

Impacted file tree graph

@@             Coverage Diff              @@
##               main     #445      +/-   ##
============================================
+ Coverage     41.33%   50.40%   +9.07%     
- Complexity     1321     1624     +303     
============================================
  Files           211      213       +2     
  Lines         10096    10269     +173     
  Branches       2028     2032       +4     
============================================
+ Hits           4173     5176    +1003     
+ Misses         5042     4101     -941     
- Partials        881      992     +111     
Impacted Files Coverage Δ
...n/androidautoidrive/carapp/maps/GMapsController.kt 0.00% <ø> (ø)
...n/androidautoidrive/carapp/maps/GMapsProjection.kt 0.00% <ø> (ø)
.../hufman/androidautoidrive/A2DPBroadcastReceiver.kt 0.00% <0.00%> (ø)
.../java/me/hufman/androidautoidrive/AddonsService.kt 0.00% <0.00%> (ø)
...an/androidautoidrive/CarAppWidgetAssetResources.kt 0.00% <ø> (ø)
.../hufman/androidautoidrive/CarConnectionListener.kt 0.00% <0.00%> (ø)
...main/java/me/hufman/androidautoidrive/CarThread.kt 0.00% <0.00%> (ø)
...java/me/hufman/androidautoidrive/DeferredUpdate.kt 84.61% <ø> (ø)
.../me/hufman/androidautoidrive/DispatcherProvider.kt 20.00% <0.00%> (+20.00%) ⬆️
...in/java/me/hufman/androidautoidrive/MainService.kt 40.07% <ø> (+40.07%) ⬆️
... and 373 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b7a3475...077ef26. Read the comment docs.

@hufman
Copy link
Collaborator

hufman commented Oct 26, 2021

It looks like a large amount of the diff is from changing the tabs to spaces. Can you configure it to keep the tabs?

@Ben2356
Copy link
Contributor Author

Ben2356 commented Oct 27, 2021

It looks like a large amount of the diff is from changing the tabs to spaces. Can you configure it to keep the tabs?

Was fighting with getting this to work correctly and not throw errors about unexpected indentation when using tabs for a while only to realize there is an outstanding issue pinterest/ktlint#1245 so sadly tabs are not playing nicely 😢

@hufman
Copy link
Collaborator

hufman commented Oct 27, 2021

Excellent find! Let's wait until those recent tab fixes are merged and see what the linter thinks after that.

@hufman
Copy link
Collaborator

hufman commented Jan 2, 2022

Can you check if the linter supports tab indentation yet? I think you'll need to force push an updated commit due to the intervening changes.

@Ben2356
Copy link
Contributor Author

Ben2356 commented Jan 3, 2022

I have actually been monitoring this to see what the status of that issue was and while it looks like they fixed the tabs now I'm encountering issues with KTLint-gradle which fails the entire linting process (JLLeitschuh/ktlint-gradle#549 (comment)) on the new version of KT that fixes those tabs. Sadly it looks like this is still blocked 😢

@hufman
Copy link
Collaborator

hufman commented Feb 21, 2022

I think I'll close this for now, because of the number of conflicting files in the intervening time.

@hufman hufman closed this Feb 21, 2022
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.

3 participants