Skip to content
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

Remove nullabel disable #307

Conversation

stefannikolei
Copy link
Contributor

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 the first nullable disable from the code base

@stefannikolei
Copy link
Contributor Author

Attention this targets the v3 branch

Copy link
Member

@JimBobSquarePants JimBobSquarePants left a comment

Choose a reason for hiding this comment

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

Oh cool for doing this! I wasn't looking forward to having to 😅

I think we should introduce Try...out in a few places since they are exceptionless.


if (converter != null)
{
return ((ICommandConverter<T>)converter).ConvertFrom(
_ = ((ICommandConverter<T>)converter).TryConvertFrom(
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't a null parsedValue be a false return?

It's been years since I've looked at this code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

here we just handle it a few lines later with a null check. Didn't want to change everything

{
if (string.IsNullOrWhiteSpace(value))
{
return Array.Empty<T>();
convertedValue = Array.Empty<T>();
Copy link
Member

Choose a reason for hiding this comment

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

Ah.... OK. So in some cases we do and some we don't.

This should be more consistant and return false on empty input.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah and in some cases we convert to an struct so there is no null..

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 gonna pull this down and have a play.

@JimBobSquarePants JimBobSquarePants force-pushed the stefannikolei/nullable/remove_nullable-disable branch from 65ecc76 to 5b6086d Compare March 5, 2023 06:09
@JimBobSquarePants
Copy link
Member

I set you on a wild goose chase there sorry!

This all looks great. Thanks! 👍

@JimBobSquarePants JimBobSquarePants merged commit 438daf0 into SixLabors:js/imagesharpV3 Mar 5, 2023
@stefannikolei
Copy link
Contributor Author

I set you on a wild goose chase there sorry!

This all looks great. Thanks! 👍

All good 😅 I left you one file with #nullable enable for you in the main project

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants