Enable nullability checks #289

JimBobSquarePants · 2023-08-22T12:33:31Z

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

Removes all #nullable disable compiler instructions except for PolygonClipper (which looks like a ton of work)
Performs minor cleanup to remove IDE warnings

codecov · 2023-08-22T12:40:43Z

Codecov Report

Merging #289 (6ef0a7b) into main (b7402f4) will increase coverage by 0%.
The diff coverage is 79%.

@@         Coverage Diff         @@
##           main   #289   +/-   ##
===================================
  Coverage    80%    80%           
===================================
  Files        97     97           
  Lines      4875   4873    -2     
  Branches    880    869   -11     
===================================
+ Hits       3925   3929    +4     
+ Misses      753    752    -1     
+ Partials    197    192    -5

Flag	Coverage Δ
unittests	`80% <79%> (+<1%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed	Coverage Δ
src/ImageSharp.Drawing/Processing/Brush.cs	`0% <0%> (ø)`
...rawing/Processing/Extensions/DrawTextExtensions.cs	`80% <ø> (ø)`
src/ImageSharp.Drawing/Processing/GradientBrush.cs	`81% <ø> (ø)`
src/ImageSharp.Drawing/Processing/ImageBrush.cs	`80% <ø> (-1%)`	⬇️
...ageSharp.Drawing/Processing/LinearGradientBrush.cs	`84% <ø> (ø)`
src/ImageSharp.Drawing/Processing/PatternBrush.cs	`75% <ø> (ø)`
src/ImageSharp.Drawing/Processing/PatternPen.cs	`50% <ø> (ø)`
...ageSharp.Drawing/Processing/RadialGradientBrush.cs	`76% <ø> (ø)`
src/ImageSharp.Drawing/Processing/RecolorBrush.cs	`87% <ø> (ø)`
src/ImageSharp.Drawing/Processing/SolidBrush.cs	`86% <ø> (ø)`
... and 21 more

... and 1 file with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

stefannikolei · 2023-09-04T05:05:10Z

src/ImageSharp.Drawing/Shapes/Path.cs

-        => this.lineSegments = segments ?? throw new ArgumentNullException(nameof(segments));
+    {
+        ArgumentNullException.ThrowIfNull(segments);
+        this.lineSegments = segments ?? throw new ArgumentNullException(nameof(segments));


Suggested change

this.lineSegments = segments ?? throw new ArgumentNullException(nameof(segments));

this.lineSegments = segments;

stefannikolei · 2023-09-04T05:10:45Z

src/ImageSharp.Drawing/Shapes/Path.cs

@@ -49,7 +48,10 @@ public Path(Path path)
    /// </summary>
    /// <param name="segments">The segments.</param>
    public Path(params ILineSegment[] segments)
-        => this.lineSegments = segments ?? throw new ArgumentNullException(nameof(segments));
+    {
+        ArgumentNullException.ThrowIfNull(segments);


I think you can even get rid of this null check. Calling this method without a parameter results in an empty array

Gave it a go... Analyzer doesn't like that!

Hm strange. My rider is not showing me an error on that

But I have more errors on my Mac.. I can not even build it correct.. Need to checkout a clean copy..

A disturbance in the force!

the reason you need the null check is you can call it like this which will result in segments being null

LineSergment[] segments = null; var path = new Path(segments)

where as if called as new Path() then an empty array will be passed for you.

A disturbance in the force!

hmm

^^

the reason you need the null check is you can call it like this which will result in segments being null

LineSergment[] segments = null; var path = new Path(segments)

where as if called as new Path() then an empty array will be passed for you.

Yeah this can happen on many path. But this will not compile when Nullable Errors are turned on.

mandel-macaque

Looks good, some minor nits regarding !=.

mandel-macaque · 2023-09-04T16:01:21Z

src/ImageSharp.Drawing/Processing/PathGradientBrush.cs

-                other.StartColor.Equals(this.StartColor) &&
-                other.EndColor.Equals(this.EndColor);
+        public bool Equals(Edge? other)
+            => other != null &&


Use is and is not over == and != since those operators can be override (and have bugs). There is an .editorconfig to do that automatically.

Yeah, I've considered this but will only enforce once I update the config. It's shared across all the projects via a Git submodule.

mandel-macaque · 2023-09-04T16:02:21Z

src/ImageSharp.Drawing/Processing/Pen.cs

@@ -23,7 +21,7 @@ namespace SixLabors.ImageSharp.Drawing.Processing;
 /// </remarks>
 public abstract class Pen : IEquatable<Pen>
 {
-    private readonly float[] pattern;
+    private readonly float[]? pattern;


Out of curiosity, since I do not know the usage. Why nullable over Array.Empty ?

I'll have a dig through actually. We might be better off changing it.

Yeah, much better!

mandel-macaque · 2023-09-04T16:02:30Z

src/ImageSharp.Drawing/Processing/Pen.cs

-    public virtual bool Equals(Pen other)
-        => this.StrokeWidth == other.StrokeWidth
+    public virtual bool Equals(Pen? other)
+        => other != null


mandel-macaque · 2023-09-04T16:07:23Z

src/ImageSharp.Drawing/Shapes/ComplexPolygon.cs

@@ -133,7 +130,7 @@ public IPath Transform(Matrix3x2 matrix)
    /// <inheritdoc />
    public IEnumerable<ISimplePath> Flatten()
    {
-        var paths = new List<ISimplePath>();
+        List<ISimplePath> paths = new();


I'm surprised you don't get a warning when declaring paths in a diff scope. Does it? (I'm doing a dry review without compiling).

No warnings. We have explicit rules requiring this. for members. I imagine the compiler understands that.

mandel-macaque · 2023-09-04T16:09:27Z

src/ImageSharp.Drawing/Utilities/ThreadLocalBlenderBuffers.cs

+        {
+            get
+            {
+                if (this.overlayBuffer != null)


JimBobSquarePants · 2023-09-05T00:55:17Z

Thanks for the reviews! Much appreciated! 🥰

JimBobSquarePants added 4 commits August 18, 2023 19:49

enable for brushes

2609d5e

enable for pens

de47366

Fix nullability for brushes and rich text rendering

b1247ae

Fix remaining nullability warnings

a44295b

JimBobSquarePants requested review from antonfirsov, tocsoft, dlemstra and brianpopow August 22, 2023 12:35

JimBobSquarePants added the codequality label Aug 22, 2023

stefannikolei reviewed Sep 4, 2023

View reviewed changes

Update Path.cs

9b9a109

mandel-macaque approved these changes Sep 4, 2023

View reviewed changes

Non-nullable stroke pattern.

6ef0a7b

JimBobSquarePants merged commit 3f1b4fa into main Sep 6, 2023

JimBobSquarePants deleted the js/nullable-enable branch September 6, 2023 05:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GitHub Sponsors

Enable nullability checks #289

Enable nullability checks #289

JimBobSquarePants commented Aug 22, 2023 •

edited

Loading

codecov bot commented Aug 22, 2023 •

edited

Loading

stefannikolei Sep 4, 2023

stefannikolei Sep 4, 2023

JimBobSquarePants Sep 4, 2023

stefannikolei Sep 4, 2023

stefannikolei Sep 4, 2023

JimBobSquarePants Sep 4, 2023

tocsoft Sep 4, 2023

stefannikolei Sep 4, 2023

stefannikolei Sep 4, 2023

mandel-macaque left a comment

mandel-macaque Sep 4, 2023

JimBobSquarePants Sep 5, 2023

mandel-macaque Sep 4, 2023

JimBobSquarePants Sep 4, 2023

JimBobSquarePants Sep 5, 2023

mandel-macaque Sep 4, 2023

mandel-macaque Sep 4, 2023

JimBobSquarePants Sep 5, 2023

mandel-macaque Sep 4, 2023

JimBobSquarePants commented Sep 5, 2023

	this.lineSegments = segments ?? throw new ArgumentNullException(nameof(segments));
	this.lineSegments = segments;

Enable nullability checks #289

Enable nullability checks #289

Conversation

JimBobSquarePants commented Aug 22, 2023 • edited Loading

Prerequisites

Description

codecov bot commented Aug 22, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mandel-macaque left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JimBobSquarePants commented Sep 5, 2023

JimBobSquarePants commented Aug 22, 2023 •

edited

Loading

codecov bot commented Aug 22, 2023 •

edited

Loading