Skip to content

Conversation

@aaronfranke
Copy link
Contributor

@aaronfranke aaronfranke commented Apr 3, 2018

I've renamed "modulus" to "remainder", added clarification for these terms, added clarification for division in C#, added a tip for how to use &, and made a few other minor tweaks.

Part of fixing #4827 but I'm not sure what to do about the Decimal docs: #4827 (comment)

@aaronfranke aaronfranke requested a review from BillWagner as a code owner April 3, 2018 05:28
@dnfclas
Copy link

dnfclas commented Apr 3, 2018

CLA assistant check
All CLA requirements met.

@BillWagner BillWagner added the ✨ 1st-time docs contributor! Indicates PRs from new contributors to the docs repository label Apr 3, 2018
Copy link
Member

@BillWagner BillWagner 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 making these updates. We really appreciate it. 👏

I've reviewed the updates. I agree with the comments made so far, and added a few suggestions.

Just @ - mention us when you've updated it, and I'll review again and merge it.

@pkulikov
Copy link
Contributor

pkulikov commented Apr 3, 2018

@BillWagner @cartermp should those two F# pages be updated with modulus to remainder changes as well?
https://docs.microsoft.com/en-us/dotnet/fsharp/language-reference/symbol-and-operator-reference/
https://docs.microsoft.com/en-us/dotnet/fsharp/language-reference/symbol-and-operator-reference/arithmetic-operators

@pkulikov
Copy link
Contributor

pkulikov commented Apr 3, 2018

@aaronfranke as for the Decimal doc changes, the description of the Modulus operator should be updated. That's pity we cannot change the name of the operator, especially if you know that this operator does the same as the Remainder method :)

If you want to update that part of documentation, make a PR with updates to the Decimal.xml file in the API reference repo

@aaronfranke
Copy link
Contributor Author

@BillWagner @pkulikov I've made several changes, let me know if there are any further concerns.

Copy link

Choose a reason for hiding this comment

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

IMO it would be preferable to use the same definition that is used in both the C# spec and the IL spec - a % b = a - (a / b) * b.

Copy link
Member

@BillWagner BillWagner Apr 4, 2018

Choose a reason for hiding this comment

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

I'm fine with either, as long as it's consistent. (I used the formula I did because it's used above as well.)

BTW, where is that formula in the C# spec? Looking here in the draft C# spec, I find:

The result of x % y is the value produced by x - (x / y) * y. If y is zero, a System.DivideByZeroException is thrown.

for integer remainder and:

z is the result of x % y and is computed as x - n * y, where n is the largest possible integer that is less than or equal to x / y.

for floating point remainder.

Copy link

Choose a reason for hiding this comment

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

BTW, where is that formula in the C# spec?

I'm not sure I understand the question. It seems that you already found it.

@cartermp
Copy link
Contributor

cartermp commented Apr 3, 2018

@pkulikov I'd approve of making similar changes to those F# docs. Absolutely feel free to include in this PR 😄

Copy link
Contributor

@pkulikov pkulikov left a comment

Choose a reason for hiding this comment

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

I've had just one remark on wording. @aaronfranke could you please also update F# articles mentioned in the comment above. I guess Visual Basic docs also might contain some modulus inclusions.

Copy link
Contributor

Choose a reason for hiding this comment

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

floating types - I'm not sure if that's correct; let's just simplify:
...use float or double type. To do that, append .0 to a number literal as the example below shows.

(Append always adds to the end by definition).

Copy link

Choose a reason for hiding this comment

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

The suggestion to append .0 is slightly misleading, that will produce a double literal, you need .0f to produce a float literal. It may be better to simply state "use floating point types" instead of going into details about how to produce a floating point literal.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mikedn @pkulikov What about "Appending .0 to a number will make it a double." ?

Copy link

Choose a reason for hiding this comment

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

I'd say that a documentation page about an operator is not the place to explain how a floating point literal is formed. Simply hinting the user to use floating point should be enough.

But then I do not know the intended target audience of this documentation. Something tells me that it may have been intended for beginners, there are too many cases where some things are over simplified to the point that they're wrong while other things are, not sure what word to use for it, accentuated, redundant etc.

I supposed that for beginners it may be worth reminding them how floating literals are formed, many do not realize that there is a difference between 2, 2.0f and 2.0.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think in this case we can omit float type mention and only refer to double as it's about having a quotient non-integer. OR: remove remark on appending: if both operands are not number literals, that remark is not correct and casting should be used.

Copy link
Member

Choose a reason for hiding this comment

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

Good discussion. I'll suggest not using .0 at all, as that works on literals, but does not on variables. For the second sentence, I'd say:

There are many ways to convert between built in numeric types

Note that I used the internal link in my comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I'll make the first part say ", use the float, double, or decimal types." I was intentionally vague with "floating types" because there is more than just float and double, as decimal exists.

@aaronfranke
Copy link
Contributor Author

aaronfranke commented Apr 4, 2018

So it looks like I should edit the F# docs too, and maybe VB ones? Anything else? As I'm not familiar with these languages I'd like to confirm they have remainder behavior.

@cartermp
Copy link
Contributor

cartermp commented Apr 4, 2018

@aaronfranke Both indeed have remainder behavior.

@BillWagner
Copy link
Member

@aaronfranke @cartermp Yes, the VB and F# docs need updating as well. You can add them to this PR, but if you don't feel comfortable in those languages, or don't have time, we'll make an issue and address those changes ourselves.

Thanks for all your work on these docs.

@aaronfranke aaronfranke requested a review from cartermp as a code owner April 4, 2018 18:57
@aaronfranke
Copy link
Contributor Author

aaronfranke commented Apr 4, 2018

I have decided to NOT edit the Visual Basic docs as I'm not familiar enough with the language and the situation seems to be a bit more complicated (the operator appears to be the text "Mod" itself).

I was, however, delighted by how easy it was to fix up the F# docs (@cartermp). I also made a few more tweaks as @mikedn @BillWagner @pkulikov suggested.

I believe this PR is good to merge. (after #4789 ?)

Copy link
Contributor

@pkulikov pkulikov left a comment

Choose a reason for hiding this comment

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

@aaronfranke that looks good to me. Thank you very much for all the effort to spot the issue, discuss it, and fix the docs at the end!

@BillWagner
Copy link
Member

Thanks @aaronfranke I created Issue #4884 for VB.

@BillWagner
Copy link
Member

@aaronfranke Thank you for sparking this discussion, participating and investigating the changes needed, and making the edits for C# and F#. We really appreciate it. 👏

Thanks for the kind words about how easy it is to participate. We're working to make it as easy as possible, and my team is glad we're on the right track. Thanks again.

I'll :shipit: now. You should see the changes on the live site in the next day or two, on our regular publishing cycle.

@BillWagner BillWagner merged commit f620153 into dotnet:master Apr 5, 2018
@BillWagner BillWagner added this to the Sprint 133 (3/17/18 - 4/06/18) milestone Apr 5, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

✨ 1st-time docs contributor! Indicates PRs from new contributors to the docs repository

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants