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.
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.
(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.
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.
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.
my goal was to avoid copying code
src/libraries/System.IO.FileSystem/tests/FileStream/FileStreamOptions.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: Stephen Toub <stoub@microsoft.com>
ghost
locked as resolved and limited conversation to collaborators
Addresses #52928 (comment)