Skip to content
This repository was archived by the owner on Oct 15, 2024. It is now read-only.

Conversation

@pt2121
Copy link
Contributor

@pt2121 pt2121 commented Oct 1, 2021

📢 Type of change

  • Bugfix
  • New feature
  • Enhancement
  • Refactoring
  • Dependency updates

📜 Description

💡 Motivation and Context

Fixes #1505

💚 How did you test it?

📝 Checklist

  • I formatted the code ./gradlew spotlessApply
  • I reviewed submitted code
  • I added a CHANGELOG entry if applicable

🔮 Next steps

📸 Screenshots / GIFs

@msfjarvis msfjarvis changed the title Replace Timber with logcat (#1505) WIP: Replace Timber with logcat Oct 1, 2021
@msfjarvis msfjarvis added A-meta Area: meta C-technical-debt Category: This makes the code harder to read and modify, but has no impact on end users P-medium Priority: medium S-waiting-on-review Status: This pull request has been implemented and needs to be reviewed labels Oct 1, 2021
@msfjarvis msfjarvis self-assigned this Oct 1, 2021
@msfjarvis
Copy link
Member

@pt2121 do you want me to review this? Draft indicates you're still working on it but I don't see any action items in your PR description.

@pt2121
Copy link
Contributor Author

pt2121 commented Oct 1, 2021

@msfjarvis I finished it late last night and wanted to do more tests. There are probably a couple things I'd like to run by you tho - will let you know.
(I will continue after work.)

@pt2121 pt2121 changed the title WIP: Replace Timber with logcat Replace Timber with logcat Oct 1, 2021
@pt2121 pt2121 marked this pull request as ready for review October 1, 2021 23:24
@pt2121 pt2121 requested a review from a team as a code owner October 1, 2021 23:24
@pt2121
Copy link
Contributor Author

pt2121 commented Oct 1, 2021

@msfjarvis This is ready for review. One small thing I want to call out is I had to manually add tags in a couple places for standalone functions.

@Skrilltrax
Copy link
Member

LGTM, I just have one suggestion. Instead of writing

logcat { "message\n${e.asLog()}" }

What if we overload the asLog to take a message parameter?

logcat { e.asLog("message") }

@msfjarvis @pt2121?

@msfjarvis
Copy link
Member

LGTM, I just have one suggestion. Instead of writing

logcat { "message\n${e.asLog()}" }

What if we overload the asLog to take a message parameter?

logcat { e.asLog("message") }

@msfjarvis @pt2121?

I'd be down for it, @pt2121 can you add the extension to app/src/main/java/dev/msfjarvis/aps/util/extensions/Extensions.kt and switch all uses to it?

@pt2121
Copy link
Contributor Author

pt2121 commented Oct 2, 2021

Yeah, will update. good idea

@pt2121
Copy link
Contributor Author

pt2121 commented Oct 3, 2021

Updated!

Copy link
Member

@Skrilltrax Skrilltrax left a comment

Choose a reason for hiding this comment

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

Looks good to me! Thanks for the pull request @pt2121

@Skrilltrax Skrilltrax merged commit 2cef6a5 into android-password-store:develop Oct 3, 2021
@msfjarvis msfjarvis mentioned this pull request Oct 3, 2021
8 tasks
@pt2121 pt2121 deleted the pt/logcat branch October 3, 2021 22:25
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

A-meta Area: meta C-technical-debt Category: This makes the code harder to read and modify, but has no impact on end users P-medium Priority: medium S-waiting-on-review Status: This pull request has been implemented and needs to be reviewed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Replace Timber with logcat

3 participants