Conversation
vmiklos
left a comment
There was a problem hiding this comment.
Hi, thanks for your first PR to plees-tracker!
Some feedback:
-
CI says testUpdate failed, does that pass for you locally? It would be good to know if that's just a flaky run or it found real badness.
-
This PR is rather large, over 1000 lines changes, which is tricky to review. Do you think you could first reduce this PR to just the "woke counter" feature, and do the manual entry one as a next PR?
-
Additional benefit to have a small first PR is that github asks me to approve the CI run for each & every attempt before you don't have a first PR accepted.
-
I see the newline style is windows in app/src/androidTest/java/hu/vmiklos/plees_tracker/ManualEntryTest.kt; the rest of the project uses unix style endlines, so that would be good to fix.
-
The project has a user guide at guide/src/usage.md, it would be good to write a paragraph about the woke counter there.
But again, just based on skimming the code quickly to provide early feedback, this looks promising. :-)
Let me know if you're open to reduce this to just the woke counter as a start or you prefer if I make a backup of this PR and then I can later also do this reducing for you.
Thanks.
|
testUpdate fails because of DatePickerDialog logic in editSleepDate. The createSleep helper function in UITestBase should create a sleep with start time 9 am and stop time 11 pm, but I dont see it while debugging. So this test might only pass when it runs after 10 am for me. I'll try to split by the pr and add to guide/src/usage.md |
Possible, it was a while ago I wrote that test. :) Let's try again. |
|
Aha, now the output is more relevant; to avoid whitespace edit wars, once you're done with the split, please run ktlint on the result, the CI log points out current cosmetic problems with the changes. Thanks. As usual, I can also do this for you if ktlint would be a pain for you; but hopefully it isn't. :-) |
No description provided.