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

Improve errors raised, simplify/generalize some code #254

Merged
merged 1 commit into from
May 24, 2024

Conversation

abelbraaksma
Copy link
Member

Just some house-keeping and getting slightly more useful errors in some cases.

if value >= 0 then
()
else
invalidArg name $"The value must be non-negative, but was {value}."
Copy link
Member

Choose a reason for hiding this comment

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

argumentoufofrange? I guess you're refactoring, which answers that...

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll double-check, but the errors raised should be the same as for similar functions in Seq.xxx. I vaguely remember being surprised about this previously. It may be because the argument check is immediate, and the range check happens while iterating the sequence (which, in turn, may have a dynamic length).

Copy link
Member Author

Choose a reason for hiding this comment

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

I actually started the refactoring to limit some allocations in an upcoming PR (and more generally, to improve the code).

Copy link
Member Author

Choose a reason for hiding this comment

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

Forgot about this discussion point. These should be invalidArg. We throw ArgumentOutOfRange if we find that, while iterating, we end up past the last index, for instance. Here, it is clearly an argument error, as there's never an index that's negative.

(this is my interpretation from MS's logic in the same/similar cases for Array/Seq/List)

@abelbraaksma abelbraaksma merged commit b5bfcaf into main May 24, 2024
6 checks passed
@abelbraaksma abelbraaksma deleted the minor-refactorings branch May 24, 2024 00:01
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