Added function for calculating positive value of number mod base.
It will be needed in further diffs to wrap value back to 0 when it overflows max value (and vice verse)
Details
Details
Typeahead without keyboard support works.
Final tests performed in last diffs.
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
Comment Actions
What do you mean that it gets the positive modulo? For number = 3 and base = -2 it returns -1
Comment Actions
Responding to both comments. I should probably call it least positive residue as defined here: https://en.wikipedia.org/wiki/Modulo_operation.
We are not concerned with negative modulus as we just need that function to "wrap" numbers between 0 and <MODULUS - 1>.
Comment Actions
Since this function is exported, someone might try to use it in the future somewhere else with a negative modulus. I think it'd be good to at least leave a comment that in such case the result will not be positive. Otherwise the code is misleading. Other that that, I agree that least positive residue is better.
That said, I don't want to block you on my opinion on naming variables. So I'm accepting, and asking other reviewers to express their opinion on whether this matters at all.
Comment Actions
You're right I'm updating the diff right now. Sorry for that, I meant to do it, but only responded to comment.
| web/utils/typeahead-utils.js | ||
|---|---|---|
| 153 ↗ | (On Diff #20413) | I think it's a typo? |
Comment Actions
It seems like this isn't typeahead-specific. Can we move it to a different place, probably in lib?
I think we need to add a couple of tests to make sure this is correct.
Comment Actions
Could you also add some tests where number is negative?
| lib/utils/math-utils.js | ||
|---|---|---|
| 3 ↗ | (On Diff #20511) | It might be a little confusing to name a parameter like a type |
Comment Actions
Changed name to dividend as it is the thing that is divided.
Added test for negative dividend.