Modify how modulo is calculated in Normal and Scientific mode.#412
Modify how modulo is calculated in Normal and Scientific mode.#412danbelcher-MSFT merged 9 commits intomicrosoft:masterfrom rudyhuyn:ModuloRemainer
Conversation
- add modrate (arithmetic modular) - modify CalcEngine to use modrate in Normal and Scientific mode and remrate in Programmer mode
|
@rudyhuyn I'd like to ask you to follow the common guidelines for writing commit messages: https://chris.beams.io/posts/git-commit/ https://gist.github.com/robertpainsi/b632364184e70900af4ab688decf6f53 and more. Basically, keep the subject line a subject. |
|
@janisozaur: I disagree and agree at the same time. There is a difference between developers working directly on a git repository and developers working indirectly on a git repository via collaboration tools (Github, Phabricator, etc...). I agree for Master/public branches.
It's perfect, you get 2 level of information, you can quickly read the history and get more details if you want. But this is not how commits work in a PR, their roles are very different, they are mainly a tool for the developer working on the PR, not especially for others. As a developer, the only field you have access to describe the changes of a commit is the commit message, you don't have another option, so it's better to be verbose and give the more information possible to reviewers to make it easier for them to understand what you did since the last time they reviewed your code. You can split your message with an empty new line, but if the "title" is So I agree for Master/public branches, github already takes care of that, but not inside a PR. I saw yesterday a comment asking a developer to squash his/her commits in a PR (It was a giant diff with more than 30 commits), it doesn't make sense, they will already be squashed once approved and merged, it's better to keep the history of all changes of the PR (in case the developer needs to roll back a change) than squashing for no real reason. |
|
Ah, I was not aware they squash the PRs. I just checked and it indeed is the case. I'll get to that in just a moment.
I disagree with your arguments, I don't see how providing an actually usable subject line in a commit depends on place where it is used. You also seem to rely on all the PRs being squashed, which should not be the case. A commit in a PR is no different outside a PR – a set of changes that are coherent and can be explained with a short subject line. Regarding squashing of PRs: I'm surprised that happens, but I have never checked. It has a number of issues, to name a few:
Have you read https://github.com/Microsoft/calculator/blob/master/CONTRIBUTING.md ? If you follow the first link to https://guides.github.com/introduction/flow/ you can also see a helpful diagram explaining the process and describing how commits are merged into master: |
|
Ugh, I just re-read my comment and noticed that part. Still, my other arguments are unaffected. |
|
I agree, squashing and GitHub have some issues (I'm not marked as the author of 4-5 commits in another Microsoft git repository because of this system while I wrote 100% of the diff...frustrating 😆). But it also makes sense and you should really assume it works this way. You owe your own forked repository, when you create a PR, you don't request to merge your branches (your forked repo will not even know) but to apply a patch based on your branch. It's a different logic and probably a better and safer one.
We are off-topic, but I will take some of your feedback into account for the future! This is sad GitHub doesn't give access to DM, feel free to ping me on one of the social network to chat if you want. |
joshkoon
left a comment
There was a problem hiding this comment.
Thanks for taking the time to make these changes, Rudy! My feedback is largely focused on ensuring the differences between mod and rem are clear for future programmers who may not be aware of the discussion and decisions that led to these changes.
…soft#412) ## Fixes microsoft#111 > The modulo operator on this calculator gives the result that is different to the most used calculators. The current `modrate` function is the equivalent of rem(...)/remainder(...), not mod(...)/modulo(...) available in some popular Math apps. ### Description of the changes: - rename `modrate` in `remrate` to be more accurate. - add `modrate`, calculating modulo similarly to Matlab, Bing, Google calculator, Maxima, Wolfram Alpha and Microsoft Excel - Add `RationalMath::Mod` using `modrate` as an alternative to `Rational::operator%` using `remrate` - Add a helper `SIGN` to retrieve the sign of a `Rational`. - modify `CalcEngine` to use `modrate` in Normal and Scientific mode and `remrate` in Programmer mode. ### How changes were validated: - manually and unit tests added
These are the changes extracted from microsoft#211 that port number's mantissa from flexible member arrays, a non-standard C++ extension, to std::vector, which enables compilation and linkage with clang. @fwcd is the original author of these changes, I (@janisozaur) merely rebased them on top of current master. The rebase was luckily fairly straight-forward, with the only real conflict coming from 7a7ceb5 (microsoft#412), but was trivial to fix.
These are the changes extracted from microsoft#211 that port number's mantissa from flexible member arrays, a non-standard C++ extension, to std::vector, which enables compilation and linkage with clang. @fwcd is the original author of these changes, I (@janisozaur) merely rebased them on top of current master. The rebase was luckily fairly straight-forward, with the only real conflict coming from 7a7ceb5 (microsoft#412), but was trivial to fix.
These are the changes extracted from microsoft#211 that port number's mantissa from flexible member arrays, a non-standard C++ extension, to std::vector, which enables compilation and linkage with clang. @fwcd is the original author of these changes, I (@janisozaur) merely rebased them on top of current master. The rebase was luckily fairly straight-forward, with the only real conflict coming from 7a7ceb5 (microsoft#412), but was trivial to fix.
These are the changes extracted from microsoft#211 that port number's mantissa from flexible member arrays, a non-standard C++ extension, to std::vector, which enables compilation and linkage with clang. @fwcd is the original author of these changes, I (@janisozaur) merely rebased them on top of current master. The rebase was luckily fairly straight-forward, with the only real conflict coming from 7a7ceb5 (microsoft#412), but was trivial to fix.
These are the changes extracted from microsoft#211 that port number's mantissa from flexible member arrays, a non-standard C++ extension, to std::vector, which enables compilation and linkage with clang. @fwcd is the original author of these changes, I (@janisozaur) merely rebased them on top of current master. The rebase was luckily fairly straight-forward, with the only real conflict coming from 7a7ceb5 (microsoft#412), but was trivial to fix.
These are the changes extracted from microsoft#211 that port number's mantissa from flexible member arrays, a non-standard C++ extension, to std::vector, which enables compilation and linkage with clang. @fwcd is the original author of these changes, I (@janisozaur) merely rebased them on top of current master. The rebase was luckily fairly straight-forward, with the only real conflict coming from 7a7ceb5 (microsoft#412), but was trivial to fix.
These are the changes extracted from microsoft#211 that port number's mantissa from flexible member arrays, a non-standard C++ extension, to std::vector, which enables compilation and linkage with clang. @fwcd is the original author of these changes, I (@janisozaur) merely rebased them on top of current master. The rebase was luckily fairly straight-forward, with the only real conflict coming from 7a7ceb5 (microsoft#412), but was trivial to fix.
These are the changes extracted from microsoft#211 that port number's mantissa from flexible member arrays, a non-standard C++ extension, to std::vector, which enables compilation and linkage with clang. @fwcd is the original author of these changes, I (@janisozaur) merely rebased them on top of current master. The rebase was luckily fairly straight-forward, with the only real conflict coming from 7a7ceb5 (microsoft#412), but was trivial to fix.
These are the changes extracted from microsoft#211 that port number's mantissa from flexible member arrays, a non-standard C++ extension, to std::vector, which enables compilation and linkage with clang. @fwcd is the original author of these changes, I (@janisozaur) merely rebased them on top of current master. The rebase was luckily fairly straight-forward, with the only real conflict coming from 7a7ceb5 (microsoft#412), but was trivial to fix.
These are the changes extracted from microsoft#211 that port number's mantissa from flexible member arrays, a non-standard C++ extension, to std::vector, which enables compilation and linkage with clang. @fwcd is the original author of these changes, I (@janisozaur) merely rebased them on top of current master. The rebase was luckily fairly straight-forward, with the only real conflict coming from 7a7ceb5 (microsoft#412), but was trivial to fix.
These are the changes extracted from microsoft#211 that port number's mantissa from flexible member arrays, a non-standard C++ extension, to std::vector, which enables compilation and linkage with clang. @fwcd is the original author of these changes, I (@janisozaur) merely rebased them on top of current master. The rebase was luckily fairly straight-forward, with the only real conflict coming from 7a7ceb5 (microsoft#412), but was trivial to fix.

Fixes #111
The current
modratefunction is the equivalent of rem(...)/remainder(...), not mod(...)/modulo(...) available in some popular Math apps.Description of the changes:
modrateinremrateto be more accurate.modrate, calculating modulo similarly to Matlab, Bing, Google calculator, Maxima, Wolfram Alpha and Microsoft ExcelRationalMath::Modusingmodrateas an alternative toRational::operator%usingremrateSIGNto retrieve the sign of aRational.CalcEngineto usemodratein Normal and Scientific mode andremratein Programmer mode.How changes were validated: