Skip to content

Add consolidated extensions from different projects#73

Merged
ax0l0tl merged 10 commits intomainfrom
feature/option-extensions
Nov 20, 2025
Merged

Add consolidated extensions from different projects#73
ax0l0tl merged 10 commits intomainfrom
feature/option-extensions

Conversation

@Momolem
Copy link
Collaborator

@Momolem Momolem commented Oct 4, 2025

We want to move the useful extensions we currently use across different projects into FS so that we don't have to copy them all the time.

@Momolem Momolem requested review from Tyrrx and ax0l0tl October 4, 2025 14:51
@Momolem Momolem self-assigned this Oct 4, 2025
… also uses to optimize common cases

test: add tests for new extensions and untested existing methods
test: test all EnumerableExtensions with a pure enumerable and with one that implements IList<T> to hit both the optimized and unoptimized branch
… flipped

refactor: change .BeautifulName extension to be more readable
test: run mutation test and add test methods to reach 95% of mutations killed
@Momolem Momolem marked this pull request as ready for review November 4, 2025 13:31
@Momolem
Copy link
Collaborator Author

Momolem commented Nov 18, 2025

From my perspective this pr can be close, @ax0l0tl @park-jasper what do you think?

string? valueTarget = "Hi";

nullTarget.ToOption(false).Should().BeNone();
nullTarget.ToOption(true).Should().BeNone();
Copy link
Member

Choose a reason for hiding this comment

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

This is weird :). I propose to remove the ToOption(bool) overloads. They unclear and inefficient. I'd always prefer a flag ? value : Option<T>.None over value.ToOption(flag). The ToOption(Func<T, bool> overloads) are fine because here the 'null results in None' semantics is clear due to non null T predicate argument.

Copy link
Member

Choose a reason for hiding this comment

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

I can see that, lets reduce it to the general case, if a user wants to then write value.ToOption(_ => true) it still carries more semantics than just using the bool parameter

Copy link
Member

@ax0l0tl ax0l0tl left a comment

Choose a reason for hiding this comment

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

Reconsider ToOption(bool) overloads, see comment in UnitTest, everything else looks good to me.

ax0l0tl and others added 2 commits November 19, 2025 08:14
…ar enough that this also does a null check, in favor of the ToOption(this T?, Func<T, bool>) extension
@ax0l0tl ax0l0tl merged commit 0e887ee into main Nov 20, 2025
2 checks passed
@ax0l0tl ax0l0tl deleted the feature/option-extensions branch November 20, 2025 10:26
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