[Kotlin Migration] SchedTest #11837
[Kotlin Migration] SchedTest #11837
Conversation
|
This looks like it's not based on latest main, as some of these changes were already made in #11802 |
Prince-kushwaha
force-pushed
the
sched
branch
4 times, most recently
from
ccc8207 to
190fb4b
Compare
lukstbit
added
the
Needs Author Reply
lukstbit
added
Needs Second Approval
|
This is an area of really fast change - which is both unfortunate (it keeps blowing up the PR, I'm so sorry) but also rare and amazing (as Damien has taken the time to make really big/important advances in the scheduler support with #10411 ) - it means this needs another scan though since I just merged #10411 so now scheduler code changed up again. I'm not aware of any other PRs against the scheduler at the moment, at least |
mikehardy
added
the
Needs Author Reply
|
And now there is a conflict, sorry |
Prince-kushwaha
force-pushed
the
sched
branch
2 times, most recently
from
7c17954 to
e4db70b
Compare
There was a problem hiding this comment.
Thanks a lot. A few nit if you please.
Your assertEqual longs, make me thought about #11965
| @@ -124,7 +123,7 @@ interface LimitMethod { | |||
| /** Given a deck, compute the number of cards to see today, taking its pre-computed limit into consideration. It | |||
| * considers either review or new cards. Used by WalkingCount to consider all subdecks and parents of a specific | |||
| * decks. */ | |||
|
|
|||
There was a problem hiding this comment.
Can you revert changes on AnkiDroid/src/main/java/com/ichi2/libanki/sched/SchedV2.java ? Or explain in the commit message why they are required (I think they are not)
| for (TreeNode<DeckDueTreeNode> node : tree) { | ||
| if (node.getValue().getDid() == didToFind) { | ||
| return node.getValue(); | ||
| for ((value) in tree) { |
There was a problem hiding this comment.
It took me some time to understand that (value) assigns value to what used to be node.getValue().
I'd prefer if we could remain closer to java and just add a KotlinCleanup to improve later.
Also, I don't like value as name here. It's far too vague. In this case, maybe something such as topLevelDeckNode would be better, since this value is one node representing a top level deck I believe
| } | ||
|
|
||
| private fun getCountsForDid(didToFind: Double): DeckDueTreeNode { |
There was a problem hiding this comment.
Can you add a kotlin cleanup. There is no reason for did to be Double. It should be a long
| // first card we get comes from parent | ||
| Card c = getCard(); | ||
| assertEquals(1, c.getDid()); | ||
| val c = card!! |
There was a problem hiding this comment.
I'm not fan of this rewritting. getCard may not be an ideal name to indicate that it actually does a non trivial access work, but at least, it's a function, so it's clear it can occur. On the other hand, c = card!! seems to be really a copy of the reference from a member to a local variable. If it ever needs to be debugged, it'll be hard to realize that some non-trivial action is actually occurring here.
So please keep getCard everywhere.
Maybe a KotlinCleanup to change getCard to some more precise name such as findNextCard()
| conf1.getJSONObject("new").put("perDay", 10) | ||
| col.decks.save(conf1) | ||
| col.reset() | ||
| assertEquals(10, col.sched.counts().new.toLong()) |
There was a problem hiding this comment.
Maybe also replace 10 by 10L so that at least it's very clear both are long.
And maybe add a comment in the commit message that you made this change because there is no assertEquals(int, int) and Kotlin rules would default to Object, Object where Java defaulted to long, long
| assertEquals("2 d", without_unicode_isolation(col.getSched().nextIvlStr(getTargetContext(), c, BUTTON_TWO))); | ||
| assertEquals("3 d", without_unicode_isolation(col.getSched().nextIvlStr(getTargetContext(), c, BUTTON_THREE))); | ||
| assertEquals("4 d", without_unicode_isolation(col.getSched().nextIvlStr(getTargetContext(), c, BUTTON_FOUR))); | ||
| assertEquals( |
There was a problem hiding this comment.
Add a cleanup to avoid code repetition here. With newlines it really becomes useful here
mikehardy
removed
the
Needs Second Approval
Pull Request template
Purpose / Description
Kotlin Migration of SchedTest