-
-
Notifications
You must be signed in to change notification settings - Fork 888
Nits #870
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Nits #870
Conversation
Codecov Report
@@ Coverage Diff @@
## master #870 +/- ##
==========================================
- Coverage 88.97% 88.97% -0.01%
==========================================
Files 1017 1017
Lines 44438 44436 -2
Branches 3228 3227 -1
==========================================
- Hits 39540 39538 -2
Misses 4176 4176
Partials 722 722
Continue to review full report at Codecov.
|
|
Ready for review. |
| /// </summary> | ||
| public static readonly CieXyz DefaultWhitePoint = Illuminants.D50; | ||
|
|
||
| private static readonly Vector3 Min = new Vector3(0, -200, 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Public fields should preceed private fields, right? I always get confused by these rules.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep (and why I reordered this one).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm surprised StyleCop hadn't already picked up on this one.
| /// <summary> | ||
| /// Endianness of a converter | ||
| /// </summary> | ||
| internal enum Endianness |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Debate on coding taste 3 .. 2 .. 1 ...
For me - fine. @JimBobSquarePants ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fine by me, simplicity always wins.
@iamcarbon Did I ever tell you how much I love you 😍 |
| /// to reduce the overhead of small incremental reads. | ||
| /// </summary> | ||
| internal class DoubleBufferedStreamReader : IDisposable | ||
| internal sealed class DoubleBufferedStreamReader : IDisposable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yes.... We should be sealing everything we can actually. There's definite advantages in the new compilers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Totally. There are some cases where it can avoid virtual calls when sealing the class. I think we've already got most of them.
|
I think this is a good chunk of stuff for this round of nits. :) |
|
@iamcarbon Just a heads up, those failed builds... Each pushed commit triggers a new build so I'm keeping an eye on things and cancelling all but the latest build each time you push to save build time. |
|
@JimBobSquarePants thanks! not sure how i missed that stylecop violation. |
This reverts commit d22f2a6.
|
success |
Prerequisites
Description
A few small nits.