Skip to content

Conversation

@SimonCropp
Copy link
Contributor

@SimonCropp SimonCropp commented Oct 25, 2022

My plan it to tackle this as small easily reviewable PRs

given the Big Bang #2236 didnt seem to work

@SimonCropp SimonCropp changed the title add some nullable ref types add some nullable ref types for image detection Oct 25, 2022
@JimBobSquarePants
Copy link
Member

My plan it to tackle this as small easily reviewable PRs

Definitely the best approach.


/// <inheritdoc/>
public IImageFormat DetectFormat(ReadOnlySpan<byte> header)
public IImageFormat? DetectFormat(ReadOnlySpan<byte> header)
Copy link
Member

Choose a reason for hiding this comment

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

I cannot decide what the best pattern for nullable return types should be. I feel like anything that returns a nullable type should probably use a TryXX(in, out..) pattern (though I still haven't found a nice async pattern for that)

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 feel like anything that returns a nullable type should probably use a TryXX(in, out..)

yeah. i was trying to do this in the least intrusive way

though I still haven't found a nice async pattern for that

neither have I

Copy link
Contributor Author

Choose a reason for hiding this comment

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

happy to add some TryXX overloads. but i would prefer to do those in a new PR

@antonfirsov
Copy link
Member

antonfirsov commented Oct 27, 2022

DetectFormat and FindFormatByFileExtension returning nullable is definitely OK. Edit: Actually I like the bool TryXyz pattern better.

Assuming we are 100% committed to finish #2231 for 3.0, we need to decide about the global strategy we want to take to refactor the entire API. The PR doesn't fit the approach recommended by @tocsoft's #2231 (comment) (changes a small-sub area without without step 1).

@SimonCropp
Copy link
Contributor Author

note that the tryxxx conversation should not be happening on this pr

@JimBobSquarePants
Copy link
Member

JimBobSquarePants commented Nov 30, 2022

Thanks for this @SimonCropp and apologies for the slow response.

I'm gonna close this in favour of the more general #2282 which allows us to complete step 1 of the global approach.

Thanks again

@SimonCropp
Copy link
Contributor Author

@JimBobSquarePants no worries at all

@SimonCropp SimonCropp deleted the nullable-ref-types-1 branch November 30, 2022 23:50
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.

3 participants