-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
move the validaiton logic to FileStreamOptions #53869
move the validaiton logic to FileStreamOptions #53869
Conversation
Tagging subscribers to this area: @carlossanlop Issue DetailsAddresses #52928 (comment)
|
{ | ||
throw new ArgumentException(SR.Format(SR.Argument_InvalidFileModeAndAccessCombo, value, _access), nameof(value)); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm. We typically consider validation that crosses properties like this to be problematic. I hadn't noticed the interactions when I suggested moving the validation logic into the setters. @bartonjs, opinions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(A happy medium might be leaving in FileStreamOptions the validation that depends only on the value being set, and put back in FileStream the rest, e.g. here we'd keep the if (value < FileMode.CreateNew || value > FileMode.Append)
check but remove the else-ifs. The validation for Share/Options/PreallocationSize/BufferSize is all fine as it's based only on the value being set.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@stephentoub done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The answer may no longer be needed, but since the question was asked:
The guidelines are that property sets shouldn't interfere with each other, but that any validation wait until a method depends on the state of those properties.
So the first if "FileMode makes no sense" makes sense here, but the remainder shouldn't happen until something wants to consume one.
If you want to keep the validation succinct, and in this type, you could add an internal Validate
method, that way the same validation is shared across all consumers.
internal static void ArgumentOutOfRangeException_Enum_Value() | ||
{ | ||
throw new ArgumentOutOfRangeException("value", SR.ArgumentOutOfRange_Enum); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this necessary? Looks like there's only a few call sites, none of which are on a hot path or in a generic context.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
my goal was to avoid copying code
src/libraries/System.IO.FileSystem/tests/FileStream/FileStreamOptions.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks.
Co-authored-by: Stephen Toub <stoub@microsoft.com>
Addresses #52928 (comment)