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
Open
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions config/detekt/detekt.yml
Original file line number Diff line number Diff line change
Expand Up @@ -590,9 +590,9 @@ style:
value: 'FIXME:'
- reason: 'Forbidden STOPSHIP todo marker in comment, please address the problem before shipping the code.'
value: 'STOPSHIP:'
# - reason: 'Forbidden TODO todo marker in comment, please do the changes.'
# value: 'TODO:'
allowedPatterns: ''
- reason: 'TODO comments must reference a GitHub issue for tracking. Format: TODO (#IssueNumber).'
value: 'TODO:'
allowedPatterns: '^TODO \\(#\\d+\\):'
anandwana001 marked this conversation as resolved.
Show resolved Hide resolved
ForbiddenImport:
active: false
imports: [ ]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ constructor(

init {
viewModelScope.launch {
// TODO: Check auth status whenever fragments resumes
// TODO(#2903): Check auth status whenever fragments resumes
anandwana001 marked this conversation as resolved.
Show resolved Hide resolved
authenticationManager.signInState.collect {
_navigationRequests.emit(onSignInStateChange(it))
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ data class LocationOfInterest(
* Converts this LOI to a mutation that can be used to update this LOI in the remote and local
* database.
*/
// TODO: Remove this test-only method
// TODO(#2903): Remove this test-only method
fun toMutation(type: Mutation.Type, userId: String): LocationOfInterestMutation =
LocationOfInterestMutation(
jobId = job.id,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ data class CaptureLocationTaskData(
val accuracy: Double?, // in metres
) : GeometryTaskData(location) {
override fun getDetailsText(): String {
// TODO: Move to strings.xml for i18n
// TODO(#2903): Move to strings.xml for i18n
anandwana001 marked this conversation as resolved.
Show resolved Hide resolved
val df = DecimalFormat("#.##")
df.roundingMode = RoundingMode.DOWN
val coordinatesString = location.coordinates.toDmsFormat()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ class MultipleChoiceTaskData(
suffix = MultipleChoiceTaskViewModel.OTHER_SUFFIX,
) ?: ""

// TODO: Make these inner classes non-static and access Task directly.
// TODO(#2903): Make these inner classes non-static and access Task directly.
anandwana001 marked this conversation as resolved.
Show resolved Hide resolved
override fun getDetailsText(): String =
selectedOptionIds
.mapNotNull { multipleChoice?.getOptionById(it) }
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ package com.google.android.ground.model.submission
* @property data A map from task id to values. This map is mutable and therefore should never be
* exposed outside this class.
*/
// TODO: Merge into Submission?
// TODO(#2903): Merge into Submission?
anandwana001 marked this conversation as resolved.
Show resolved Hide resolved
data class SubmissionData(private val data: Map<String, TaskData?> = mapOf()) {

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,8 +36,9 @@ constructor(
) {

/**
* Task type names as they appear in the remote db, but in uppercase. DO NOT RENAME! TODO: Define
* these in data layer!
* Task type names as they appear in the remote db, but in uppercase. DO NOT RENAME!
anandwana001 marked this conversation as resolved.
Show resolved Hide resolved
*
* TODO(#2903): Define these in data layer!
*/
enum class Type {
UNKNOWN,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ internal object ValueJsonConverter {
private fun toJsonArray(response: MultipleChoiceTaskData): JSONArray =
JSONArray().apply { response.selectedOptionIds.forEach { this.put(it) } }

// TODO: Replace with proto conversion logic if this is still necessary
// TODO(#2903): Replace with proto conversion logic if this is still necessary
anandwana001 marked this conversation as resolved.
Show resolved Hide resolved
fun toResponse(task: Task, obj: Any): TaskData? {
if (JSONObject.NULL === obj) {
return null
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,8 @@ import kotlinx.coroutines.CancellationException
import kotlinx.coroutines.tasks.await
import timber.log.Timber

// TODO: Add column to Submission table for storing uploaded media urls
// TODO: Synced to remote db as well
// TODO(#2903): Add column to Submission table for storing uploaded media urls
anandwana001 marked this conversation as resolved.
Show resolved Hide resolved
// TODO(#2903): Synced to remote db as well
anandwana001 marked this conversation as resolved.
Show resolved Hide resolved
@Singleton
class FirebaseStorageManager @Inject constructor() : RemoteStorageManager {

Expand Down Expand Up @@ -61,7 +61,7 @@ class FirebaseStorageManager @Inject constructor() : RemoteStorageManager {
*
* user-media/surveys/{survey_id}/submissions/{field_id-uuid.jpg}
*/
// TODO: Refactor this into MediaStorageRepository.
// TODO(#2903): Refactor this into MediaStorageRepository.
anandwana001 marked this conversation as resolved.
Show resolved Hide resolved
fun getRemoteMediaPath(surveyId: String, filename: String): String =
StringJoiner(File.separator)
.add(MEDIA_ROOT_DIR)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,6 @@ open class FluentFirestore protected constructor(private val db: FirebaseFiresto

protected fun db(): FirebaseFirestore = db

// TODO: Wrap in fluent version of WriteBatch.
// TODO(#2903): Wrap in fluent version of WriteBatch.
anandwana001 marked this conversation as resolved.
Show resolved Hide resolved
fun batch(): WriteBatch = db.batch()
}
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ import com.google.protobuf.timestamp
import java.util.Date
import kotlinx.collections.immutable.toImmutableMap

// TODO: Add test coverage
// TODO(#2903): Add test coverage
anandwana001 marked this conversation as resolved.
Show resolved Hide resolved
fun SubmissionMutation.createSubmissionMessage(user: User) = submission {
assert(userId == user.id) { "UserId doesn't match: expected $userId, found ${user.id}" }

Expand Down Expand Up @@ -127,7 +127,7 @@ fun LocationOfInterestMutation.createLoiMessage(user: User) = locationOfInterest
}

private fun toTaskData(id: String, newTaskData: TaskData) = taskData {
// TODO: What should be the ID?
// TODO(#2903): What should be the ID?
anandwana001 marked this conversation as resolved.
Show resolved Hide resolved
taskId = id

when (newTaskData) {
Expand All @@ -147,7 +147,7 @@ private fun toTaskData(id: String, newTaskData: TaskData) = taskData {
newTaskData.altitude?.let { altitude = it }
newTaskData.accuracy?.let { accuracy = it }
coordinates = newTaskData.location.coordinates.toMessage()
// TODO: Add timestamp
// TODO(#2903): Add timestamp
anandwana001 marked this conversation as resolved.
Show resolved Hide resolved
}
is GeometryTaskData -> drawGeometryResult = drawGeometryResult {
geometry = newTaskData.geometry.toMessage()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ import com.google.android.ground.proto.LocationOfInterest as LocationOfInterestP
import com.google.android.ground.proto.LocationOfInterest.Source
import com.google.firebase.firestore.DocumentSnapshot

// TODO: Add tests.
// TODO(#2903): Add tests.
anandwana001 marked this conversation as resolved.
Show resolved Hide resolved
/** Converts between Firestore documents and [LocationOfInterest] instances. */
object LoiConverter {
// TODO(#2392): Define field names on DocumentReference objects, not converters.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,13 +49,13 @@ constructor(

private val locationCallback = LocationSharedFlowCallback(_locationUpdates, externalScope)

// TODO: Request updates on resume.
// TODO(#2903): Request updates on resume.
anandwana001 marked this conversation as resolved.
Show resolved Hide resolved
/** Immediately emits the last known location (if any) and then subscribes to location updates. */
suspend fun requestLocationUpdates() {
locationClient.getLastLocation()?.let { _locationUpdates.emit(it) }
locationClient.requestLocationUpdates(FINE_LOCATION_UPDATES_REQUEST, locationCallback)
}

// TODO: Remove updates on pause.
// TODO(#2903): Remove updates on pause.
anandwana001 marked this conversation as resolved.
Show resolved Hide resolved
suspend fun disableLocationUpdates() = locationClient.removeLocationUpdates(locationCallback)
}
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ constructor(
}

private fun getGoogleSignInClient(activity: Activity): GoogleSignInClient =
// TODO: Use app context instead of activity?
// TODO(#2903): Use app context instead of activity?
anandwana001 marked this conversation as resolved.
Show resolved Hide resolved
GoogleSignIn.getClient(activity, googleSignInOptions)

private fun onActivityResult(activityResult: ActivityResult) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ class IconFactory @Inject constructor(@ApplicationContext private val context: C
/** Returns a [BitmapDescriptor] for representing an individual marker on the map. */
fun getMarkerIcon(color: Int, scale: Float): BitmapDescriptor {
val bitmap = getMarkerBitmap(color, scale)
// TODO: Cache rendered bitmaps.
// TODO(#2903): Cache rendered bitmaps.
anandwana001 marked this conversation as resolved.
Show resolved Hide resolved
return BitmapDescriptorFactory.fromBitmap(bitmap)
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,8 @@ constructor(
pinColor = job.getDefaultColor()

// Drop a marker for current value
// TODO: The restored marker appears to be slightly shifted. Check for accuracy of lat/lng.
// TODO(#2903): The restored marker appears to be slightly shifted. Check for accuracy of
anandwana001 marked this conversation as resolved.
Show resolved Hide resolved
// lat/lng.
(taskData as? DropPinTaskData)?.let { dropMarker(it.location) }
}

Expand Down Expand Up @@ -80,7 +81,7 @@ constructor(
id = uuidGenerator.generateUuid(),
type = FeatureType.USER_POINT.ordinal,
geometry = point,
// TODO: Set correct pin color.
// TODO(#2903): Set correct pin color.
anandwana001 marked this conversation as resolved.
Show resolved Hide resolved
style = Feature.Style(pinColor),
clusterable = false,
selected = true,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ import kotlinx.coroutines.launch
class HomeScreenFragment :
AbstractFragment(), BackPressListener, NavigationView.OnNavigationItemSelectedListener {

// TODO: It's not obvious which locations of interest are in HomeScreen vs MapContainer;
// TODO(#2903): It's not obvious which locations of interest are in HomeScreen vs MapContainer;
anandwana001 marked this conversation as resolved.
Show resolved Hide resolved
// make this more intuitive.

@Inject lateinit var ephemeralPopups: EphemeralPopups
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ internal constructor(
return null
}

// TODO: Check whether the previous user id matches with current user or not.
// TODO(#2903): Check whether the previous user id matches with current user or not.
anandwana001 marked this conversation as resolved.
Show resolved Hide resolved
return draft
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -127,9 +127,9 @@ internal constructor(

init {
// THIS SHOULD NOT BE CALLED ON CONFIG CHANGE
// TODO: Clear location of interest markers when survey is deactivated.
// TODO: Since we depend on survey stream from repo anyway, this transformation can be moved
// into the repository.
// TODO(#2903): Clear location of interest markers when survey is deactivated.
anandwana001 marked this conversation as resolved.
Show resolved Hide resolved
// TODO(#2903): Since we depend on survey stream from repo anyway, this transformation can be
anandwana001 marked this conversation as resolved.
Show resolved Hide resolved
// moved into the repository.

@OptIn(FlowPreview::class)
mapLoiFeatures =
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,7 @@ class GoogleMapsFragment : SupportMapFragment(), MapFragment {
get() = map.cameraPosition.zoom

private fun onApplyWindowInsets(view: View, insets: WindowInsetsCompat): WindowInsetsCompat {
// TODO: Move extra padding to dimens.xml.
// TODO(#2903): Move extra padding to dimens.xml.
anandwana001 marked this conversation as resolved.
Show resolved Hide resolved
// HACK: Fix padding when keyboard is shown; we limit the padding here to prevent the
// watermark from flying up too high due to the combination of translateY and big inset
// size due to keyboard.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -177,7 +177,7 @@ class MogClient(val collection: MogCollection, val remoteStorageManager: RemoteS
path: MogPathOrUrl,
mogBounds: TileCoordinates,
): Deferred<MogMetadata?> = runBlocking {
// TODO: Exceptions get propagated as cancellation of the coroutine. Handle them!
// TODO(#2903): Exceptions get propagated as cancellation of the coroutine. Handle them!
Copy link
Collaborator

Choose a reason for hiding this comment

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

@shobhitagarwal1612 Do you know what this is referring to? Still valid?

async { path.toUrl()?.readMetadata(mogBounds) }.also { cache.put(path, it) }
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ data class MogImageMetadata(
private val tileCountX = imageWidth / tileWidth
private val tileCountY = imageLength / tileLength

// TODO: Verify X and Y scales are the same.
// TODO(#2903): Verify X and Y scales are the same.
anandwana001 marked this conversation as resolved.
Show resolved Hide resolved
val zoom = originTile.zoom

fun hasTile(x: Int, y: Int) =
Expand Down Expand Up @@ -80,7 +80,7 @@ data class MogImageMetadata(
originTile,
tiffTagToValue[TiffTag.TileWidth] as Int,
tiffTagToValue[TiffTag.TileLength] as Int,
// TODO: Refactor casts into typed accessors.
// TODO(#2903): Refactor casts into typed accessors.
anandwana001 marked this conversation as resolved.
Show resolved Hide resolved
tiffTagToValue[TiffTag.TileOffsets] as List<Long>,
tiffTagToValue[TiffTag.TileByteCounts] as List<Long>,
tiffTagToValue[TiffTag.ImageWidth] as Int,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,12 +30,12 @@ private const val NULL_CHAR = 0.toChar()
class MogMetadataReader(private val seekable: SeekableInputStream) {
private lateinit var dataInput: DataInput

// TODO: Refactor Map into its own class.
// TODO(#2903): Refactor Map into its own class.
anandwana001 marked this conversation as resolved.
Show resolved Hide resolved
fun readImageFileDirectories(): List<Map<TiffTag, Any?>> {
val byteOrderCode = readByteOrderString()
dataInput = createDataInput(byteOrderCode)

// TODO: Add support for BigTIFF.
// TODO(#2903):\\\ Add support for BigTIFF.
anandwana001 marked this conversation as resolved.
Show resolved Hide resolved
val fileIdentifier = dataInput.readUnsignedShort()
validateFileIdentifier(fileIdentifier)

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,5 +36,5 @@ To simplify discovery and retrieval, MOGs are organized in collections structure
See [Map and Tile Coordinates](https://developers.google.com/maps/documentation/android-sdk/coordinates)
in the Google Maps Platform docs for info.

<!-- TODO: Provide example usage. -->
<!-- TODO: Provide illustration. -->
<!-- TODO(#2903): Provide example usage. -->
anandwana001 marked this conversation as resolved.
Show resolved Hide resolved
<!-- TODO(#2903): Provide illustration. -->
anandwana001 marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,7 @@ class OfflineAreaSelectorFragment : AbstractMapContainerFragment() {
AppTheme {
DownloadProgressDialog(
progress = progress.value,
// TODO - Add Download Cancel Feature
// TODO(#2903): - Add Download Cancel Feature
anandwana001 marked this conversation as resolved.
Show resolved Hide resolved
// https://github.com/google/ground-android/issues/2884
onDismiss = { openAlertDialog.value = false },
)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ import com.google.android.ground.R
* NOTE: It uses [PreferenceFragmentCompat] instead of [ ], so dagger can't inject into it like it
* does in other fragments.
*
* TODO: Create dagger module and support injection into this fragment.
* TODO(#2903): Create dagger module and support injection into this fragment.
anandwana001 marked this conversation as resolved.
Show resolved Hide resolved
*/
class SettingsFragment : PreferenceFragmentCompat(), Preference.OnPreferenceClickListener {

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ class SurveySelectorFragment : AbstractFragment(), BackPressListener {
binding.recyclerView.adapter = adapter
getAbstractActivity().setSupportActionBar(binding.toolbar)

// TODO - https://github.com/google/ground-android/issues/2692#issuecomment-2430978043
// TODO(#2903): - https://github.com/google/ground-android/issues/2692#issuecomment-2430978043
anandwana001 marked this conversation as resolved.
Show resolved Hide resolved
if (parentFragmentManager.backStackEntryCount > 0) {
getAbstractActivity().supportActionBar?.setDisplayHomeAsUpEnabled(true)
} else {
Expand Down
2 changes: 1 addition & 1 deletion ground/src/main/res/layout/main_act.xml
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@
Translucent scrim to make status bar legible when shown overlaid on map or toolbar.
Workaround for possible bug in Navigation Architecture Components where translucent system
bars get replaced with solid white background in fragment after navigation.
TODO: Is this a bug? If so, what's the bug ID? Has it been fixed?
TODO(#2903): Is this a bug? If so, what's the bug ID? Has it been fixed?
anandwana001 marked this conversation as resolved.
Show resolved Hide resolved
-->
<View
android:id="@+id/status_bar_scrim"
Expand Down
2 changes: 1 addition & 1 deletion ground/src/main/res/layout/settings_activity.xml
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@
Translucent scrim to make status bar legible when shown overlaid on map or toolbar.
Workaround for possible bug in Navigation Architecture Components where translucent system
bars get replaced with solid white background in fragment after navigation.
TODO: Is this a bug? If so, what's the bug ID? Has it been fixed?
TODO(#2903): Is this a bug? If so, what's the bug ID? Has it been fixed?
anandwana001 marked this conversation as resolved.
Show resolved Hide resolved
-->

<View
Expand Down
2 changes: 1 addition & 1 deletion ground/src/main/res/navigation/nav_graph.xml
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
~ limitations under the License.
-->

<!-- TODO: Use camelCase for all ids here for consistency -->
<!-- TODO(#2903): Use camelCase for all ids here for consistency -->
anandwana001 marked this conversation as resolved.
Show resolved Hide resolved

<navigation xmlns:android="http://schemas.android.com/apk/res/android"
xmlns:app="http://schemas.android.com/apk/res-auto"
Expand Down
2 changes: 1 addition & 1 deletion ground/src/main/res/values/dimens.xml
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@

<!-- Task selection tabs -->
<dimen name="map_btn_margin_bottom">48dp</dimen>
<!-- TODO: Make dimensions relative using Android data binding. -->
<!-- TODO(#2903): Make dimensions relative using Android data binding. -->
anandwana001 marked this conversation as resolved.
Show resolved Hide resolved
<dimen name="toolbar_elevation">4dp</dimen>

<!-- Navigation drawer -->
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -198,7 +198,7 @@ class MediaUploadWorkerTest : BaseHiltTest() {
)
}

// TODO: Replace all this w/ FakeData functions that return good base model objects.
// TODO(#2903): Replace all this w/ FakeData functions that return good base model objects.
anandwana001 marked this conversation as resolved.
Show resolved Hide resolved
companion object {
private const val PHOTO_TASK_ID = "photo_task_id"
private val TEST_PHOTO_TASK: Task =
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ abstract class BaseTaskFragmentTest<F : AbstractTaskFragment<VM>, VM : AbstractT

/** Asserts that the task fragment has the given list of buttons in the exact same order. */
protected fun assertFragmentHasButtons(vararg buttonActions: ButtonAction) {
// TODO: Also verify the visibility/state of the button
// TODO(#2903): Also verify the visibility/state of the button
anandwana001 marked this conversation as resolved.
Show resolved Hide resolved
assertThat(fragment.buttonDataList.map { it.button.getAction() })
.containsExactlyElementsIn(buttonActions)
buttonActions.withIndex().forEach { (index, expected) ->
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ import org.junit.runner.RunWith
import org.mockito.Mock
import org.robolectric.RobolectricTestRunner

// TODO: Add a test for selecting a date and verifying response.
// TODO(#2903): Add a test for selecting a date and verifying response.
anandwana001 marked this conversation as resolved.
Show resolved Hide resolved

@HiltAndroidTest
@RunWith(RobolectricTestRunner::class)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -197,7 +197,7 @@ class DrawAreaTaskFragmentTest :
private fun updateLastVertexAndAddPoint(coordinate: Coordinates) {
updateLastVertex(coordinate, false)

// TODO: Refactor and move one level up with the rest of the runner logic.
// TODO(#2903): Refactor and move one level up with the rest of the runner logic.
anandwana001 marked this conversation as resolved.
Show resolved Hide resolved
runner().clickButton("Add point")
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ import org.mockito.Mockito.doNothing
import org.mockito.Mockito.mock
import org.robolectric.RobolectricTestRunner

// TODO: Convert to fragment test for better coverage
// TODO(#2903): Convert to fragment test for better coverage
anandwana001 marked this conversation as resolved.
Show resolved Hide resolved

@HiltAndroidTest
@RunWith(RobolectricTestRunner::class)
Expand Down
Loading
Loading