NF: Correct reps documentation
#17964
Conversation
Rename it, it's deprecated upstream: https://github.com/ankitects/anki/blob/038d85b1d9e1896e93a3e4a26f600c79ddc33611/pylib/anki/scheduler/v3.py#L43-L44 I'd prefer something which relates it to its intended usage in the timebox. |
david-allison
added
Needs Author Reply
|
Seriously. |
Arthur-Milchior
force-pushed
the
reps
branch
from
206c405 to
183afdd
Compare
Arthur-Milchior
removed
the
Needs Author Reply
| */ | ||
| var reps: Int = 0 | ||
| var numberOfAnswerRecorded: Int = 0 |
There was a problem hiding this comment.
I'm not an english native, but I find this a little odd to read. Not blocking, but I'll leave the final decision of merging to somebody else
There was a problem hiding this comment.
numberOfAnswersRecorded is the correction
@BrayanDSO does the fix still sound odd? recordedAnswerCount preferred?
There was a problem hiding this comment.
numberOfAnswersRecorded is the correct one to my "ears"
There was a problem hiding this comment.
@Arthur-Milchior just no getting this through without little naming battles 😆
If you push a commit that uses numberOfAnswersRecorded I'd say merge at will
mikehardy
added
Needs Author Reply
`reps` used to have a different meaning than today. I'd actually love to rename this variable, but it's the name used in upstream, so I guess it would not be accepted. I considered adding more information, such as the fact that in practice, it mostly means that this value is reset when ankidroid restart, with maybe some exceptions. Or that we should only care about the difference between values at different point in time and not about the actual value of this variable. But actually it does not matter. It's only used for timebox and has 3 occurrences in the codebase. Any more documentation would be overkill. Fixed ankidroid#6830. Well actually, it was fixed in 7a65160 long ago, but the documen tation did not reflect that. So let's claim I just fixed a 5 yo bug!
There was a problem hiding this comment.
I believe if there are naming nits and they are the only thing holding something back, it's okay to just finish them and push them and merge, so Arthur hopefully you agree - I pushed a change with the consensus numberOfAnswersRecorded name and I'm queueing this for merge
so none of us have to pay any more attention to it :-)
Cheers
mikehardy
removed
the
Needs Author Reply
github-actions
bot
removed
the
Pending Merge
repsused to have a different meaning than today. I'd actually love to rename this variable, but it's the name used in upstream, so I guess it would not be accepted.I considered adding more information, such as the fact that in practice, it mostly means that this value is reset when ankidroid restart, with maybe some exceptions. Or that we should only care about the difference between values at different point in time and not about the actual value of this variable. But actually it does not matter. It's only used for timebox and has 3 occurrences in the codebase. Any more documentation would be overkill.
Fixed #6830. Well actually, it was fixed in
7a65160 long ago, but the documen tation did not reflect that. So let's claim I just fixed a 5 yo bug!