Skip to content

Modify how modulo is calculated in Normal and Scientific mode.#412

Merged
danbelcher-MSFT merged 9 commits intomicrosoft:masterfrom
rudyhuyn:ModuloRemainer
Apr 17, 2019
Merged

Modify how modulo is calculated in Normal and Scientific mode.#412
danbelcher-MSFT merged 9 commits intomicrosoft:masterfrom
rudyhuyn:ModuloRemainer

Conversation

@rudyhuyn
Copy link
Copy Markdown
Contributor

Fixes #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

- add modrate (arithmetic modular)
- modify CalcEngine to use modrate in Normal and Scientific mode and remrate in Programmer mode
@janisozaur
Copy link
Copy Markdown
Contributor

@rudyhuyn
Copy link
Copy Markdown
Contributor Author

rudyhuyn commented Mar 30, 2019

@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.
But all commits from a PR are squashed once merged in Master by Github, the subject of the commit becoming the title of the PR. So when you look at the history you have:

  • commit subjects: short title (in fact the title of the PR associated)
  • a preview of the detailed description of the PR associated.

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 improvements, in fact, it gives even less information than a bullet list.

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.

@janisozaur
Copy link
Copy Markdown
Contributor

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.

This is not how commits work in a PR, their roles are very different.

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 ?

https://github.com/Microsoft/calculator/blob/8a75dcd09d6630ac24e049af9ebb1164fdfac9bf/CONTRIBUTING.md#L52-L53

https://github.com/Microsoft/calculator/blob/8a75dcd09d6630ac24e049af9ebb1164fdfac9bf/CONTRIBUTING.md#L57-L62

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:
image

@janisozaur
Copy link
Copy Markdown
Contributor

@rudyhuyn
Copy link
Copy Markdown
Contributor Author

rudyhuyn commented Mar 30, 2019

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.

  • better: shorter history (without thousands of commits named "take feedback into account", "fix typo", "add unit tests"
  • safer: 100% of the code visible in the history is approved and stamped by the owner of the repo (Microsoft), they become the official owner and are responsible in case of copyright/pattern infringement, so better to squash everything to to take the ownership of the "sub-commits" not reviewed.

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.

Copy link
Copy Markdown
Member

@joshkoon joshkoon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

joshkoon
joshkoon previously approved these changes Apr 1, 2019
Copy link
Copy Markdown

@danbelcher-MSFT danbelcher-MSFT left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, Rudy!

@danbelcher-MSFT danbelcher-MSFT merged commit 7a7ceb5 into microsoft:master Apr 17, 2019
EriWong pushed a commit to EriWong/calculator that referenced this pull request Jun 5, 2019
…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
janisozaur pushed a commit to janisozaur/calculator that referenced this pull request Jun 6, 2019
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.
janisozaur pushed a commit to janisozaur/calculator that referenced this pull request Jun 10, 2019
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.
janisozaur pushed a commit to janisozaur/calculator that referenced this pull request Jun 11, 2019
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.
janisozaur pushed a commit to janisozaur/calculator that referenced this pull request Jun 11, 2019
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.
janisozaur pushed a commit to janisozaur/calculator that referenced this pull request Jun 19, 2019
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.
janisozaur pushed a commit to janisozaur/calculator that referenced this pull request Jun 20, 2019
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.
janisozaur pushed a commit to janisozaur/calculator that referenced this pull request Aug 2, 2019
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.
janisozaur pushed a commit to janisozaur/calculator that referenced this pull request Dec 10, 2019
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.
janisozaur pushed a commit to janisozaur/calculator that referenced this pull request Dec 10, 2019
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.
janisozaur pushed a commit to janisozaur/calculator that referenced this pull request Oct 16, 2021
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.
janisozaur pushed a commit to janisozaur/calculator that referenced this pull request Oct 16, 2021
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Modulo operator should work like other calculators out there

6 participants