-
Notifications
You must be signed in to change notification settings - Fork 532
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
Fix #3317: Update old todos #3610
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM for codeowner files!
Assigning @BenHenning for code owner reviews. Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code owner files LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM for #1104's to-do change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved for code-owner files. Thanks
@BenHenning I and @prayutsu did some research on the TODO for issue #322, but we weren't able to reach a conclusion. It looks like it needs a deeper investigation. Repurposed the current TODO to a new issue so that this doesn't block this PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @Sparsh1212. Generally LGTM, just looking to get confirmation from two other team members before approving these updates.
It's great to see the TODOs becoming up-to-date & correctly formatted. :)
app/src/sharedTest/java/org/oppia/android/app/testing/NavigationDrawerActivityProdTest.kt
Show resolved
Hide resolved
data/src/main/java/org/oppia/android/data/backends/gae/model/GaeFeedbackReport.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM for issue #3016! Thanks for your patience
@Sparsh1212 let's file a new issue to track the work for #114's TODO. |
@BenHenning Filed new issue #3646 as a replacement for #114. PTAL. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @Sparsh1212! LGTM.
Merging since the Bazel tests should be fine, and they seem to be hanging at the moment. |
Explanation
Fixes #3317: Correct poorly formatted todos and update those which doesn't correspond to an open issue.
Issues that we should reopen because they aren't resolved, or issues which have additional work to complete:
Issues whose TODOs need to be repurposed to new/other issues:
Issues whose TODOs are also resolved and thus should be removed from code, or TODOs which do not need to be resolved:
Checklist