Skip to content

Conversation

@iamcarbon
Copy link
Contributor

@iamcarbon iamcarbon commented Apr 1, 2019

Prerequisites

  • I have written a descriptive pull-request title
  • I have verified that there are no overlapping pull-requests open
  • I have verified that I am following matches the existing coding patterns and practice as demonstrated in the repository. These follow strict Stylecop rules 👮.
  • I have provided test coverage for my change (where applicable)

Description

A few small nits.

  • Remove unused namespaces
  • Add missing periods in documentation
  • Replace Endianness enum with bool
  • Use => to format a few shorter method bodies
  • Use MathF instead of casting

@codecov
Copy link

codecov bot commented Apr 1, 2019

Codecov Report

Merging #870 into master will decrease coverage by <.01%.
The diff coverage is 77.55%.

Impacted file tree graph

@@            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
Impacted Files Coverage Δ
src/ImageSharp/Advanced/AdvancedImageExtensions.cs 82.35% <ø> (ø) ⬆️
src/ImageSharp/Image.WrapMemory.cs 100% <ø> (ø) ⬆️
...arp/Formats/Jpeg/Components/Decoder/FastACTable.cs 95% <ø> (ø) ⬆️
src/ImageSharp/IO/LocalFileSystem.cs 100% <ø> (ø) ⬆️
...Sharp/Formats/Jpeg/Components/Decoder/JpegFrame.cs 100% <ø> (ø) ⬆️
...orSpaces/Conversion/VonKriesChromaticAdaptation.cs 100% <ø> (ø) ⬆️
...Spaces/Conversion/ColorSpaceConverter.LinearRgb.cs 33.09% <ø> (ø) ⬆️
src/ImageSharp/ColorSpaces/Cmyk.cs 94.73% <ø> (ø) ⬆️
src/ImageSharp/Primitives/LongRational.cs 92.18% <ø> (ø) ⬆️
...olorConverters/JpegColorConverter.FromYCbCrSimd.cs 84.9% <ø> (ø) ⬆️
... and 50 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c2d2ccb...3f78fe9. Read the comment docs.

@iamcarbon
Copy link
Contributor Author

Ready for review.

/// </summary>
public static readonly CieXyz DefaultWhitePoint = Illuminants.D50;

private static readonly Vector3 Min = new Vector3(0, -200, 0);
Copy link
Member

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.

Copy link
Contributor Author

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).

Copy link
Member

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
Copy link
Member

@antonfirsov antonfirsov Apr 1, 2019

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 ?

Copy link
Member

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.

@JimBobSquarePants
Copy link
Member

Add missing periods in documentation

@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
Copy link
Member

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.

Copy link
Contributor Author

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.

@iamcarbon
Copy link
Contributor Author

I think this is a good chunk of stuff for this round of nits. :)

@JimBobSquarePants
Copy link
Member

@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.

@CLAassistant
Copy link

CLAassistant commented Apr 2, 2019

CLA assistant check
All committers have signed the CLA.

@iamcarbon
Copy link
Contributor Author

@JimBobSquarePants thanks! not sure how i missed that stylecop violation.

@iamcarbon
Copy link
Contributor Author

iamcarbon commented Apr 2, 2019

success

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants