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

Automate the TODO format check and fix older format #2904

Open
wants to merge 20 commits into
base: master
Choose a base branch
from

Conversation

anandwana001
Copy link
Collaborator

@anandwana001 anandwana001 commented Dec 9, 2024

@anandwana001 anandwana001 changed the title Added single todo ticket for untracked todos Automate the TODO format check and fix older format Dec 9, 2024
Copy link

codecov bot commented Dec 9, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 62.52%. Comparing base (a8cbc36) to head (7b3975f).

Additional details and impacted files
@@            Coverage Diff            @@
##             master    #2904   +/-   ##
=========================================
  Coverage     62.52%   62.52%           
  Complexity     1203     1203           
=========================================
  Files           267      267           
  Lines          6357     6357           
  Branches        881      881           
=========================================
  Hits           3975     3975           
  Misses         1831     1831           
  Partials        551      551           
Files with missing lines Coverage Δ
.../src/main/java/com/google/android/ground/Config.kt 100.00% <ø> (ø)
...ain/java/com/google/android/ground/MainActivity.kt 47.72% <ø> (ø)
...in/java/com/google/android/ground/MainViewModel.kt 75.51% <ø> (ø)
...m/google/android/ground/model/geometry/Geometry.kt 71.69% <ø> (ø)
...in/java/com/google/android/ground/model/job/Job.kt 59.25% <ø> (ø)
...und/model/locationofinterest/LocationOfInterest.kt 90.00% <ø> (ø)
...ground/model/submission/CaptureLocationTaskData.kt 50.00% <ø> (ø)
...android/ground/model/submission/DropPinTaskData.kt 40.00% <ø> (ø)
...ndroid/ground/model/submission/GeometryTaskData.kt 11.11% <ø> (ø)
.../ground/model/submission/MultipleChoiceTaskData.kt 64.51% <ø> (ø)
... and 42 more

Copy link
Collaborator

@gino-m gino-m left a comment

Choose a reason for hiding this comment

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

Thanks @anandwana001 for adding the detekt check. A single ticket isn't really helpful for tracking those TODO, though - ideally we'd have a ticket for each issue or class of issues.

Also note that I've started adding the full URL of the ticket to the TODO() tag so that they're clickable in Android Studio. That will usually cause the TODO to wrap onto the next line, in which case you can intent two spaces to ensure AS interprets the second line as part of the same TODO.

@anandwana001
Copy link
Collaborator Author

anandwana001 commented Dec 10, 2024

@gino-m
The goal of this PR:

  1. To enable the TODO format check
  2. All the untracked TODO, will be connected with a single issue [Code health] Untracked TODOs #2903, where any developer in the future can create sub-tickets and work on it.
  3. As per the PR, there are 38 untracked issues, for which we have to create individual tickets with all details, might take time, and block us from enabling the TODO format check

Hence this PR.

Copy link
Collaborator

@gino-m gino-m left a comment

Choose a reason for hiding this comment

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

I've added links to appropriate issues, creating them where appropriate.

Idea: Can we use https://github.com/marketplace/actions/todo-to-issue to auto-generate issues for each TODO as they're added in the future? In that case we'd have a different format for TODO URLs:

// TODO: Todo description
// Issue URL: ...github url.

@shobhitagarwal1612 @scolsen Wdyt? If you agree I can enable the plugin and try it out.
Wdyt? If you like the idea I can enable the plugin for you to try out.

@gino-m
Copy link
Collaborator

gino-m commented Dec 12, 2024

We spoke.

  • @shobhitagarwal1612 will set up the GitHub action to create issues on merge to master.
  • @anandwana001 Please move URLs to 2nd line as "Issue URL" (see above) in the PR and throughout the whole codebase.
  • @anandwana001 Also update check to require TODOs to look like // TODO: ...

Copy link
Collaborator

@gino-m gino-m left a comment

Choose a reason for hiding this comment

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

Once nit, otherwise LGTM

config/detekt/detekt.yml Outdated Show resolved Hide resolved
ground/build.gradle Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants