Opening a file in Read mode now returns a read-only stream#260
Opening a file in Read mode now returns a read-only stream#260fgreinacher merged 5 commits intoTestableIO:masterfrom Shtong:Fix230
Conversation
| if (access == FileAccess.Read) | ||
| streamType = MockFileStream.StreamType.READ; | ||
| else if (mode == FileMode.Append) | ||
| streamType = MockFileStream.StreamType.APPEND; |
There was a problem hiding this comment.
It looks like we've lost the stream.Seek call for the Append scenario?
There was a problem hiding this comment.
Seeking is already handled by the MockFileStream constructor called later
|
|
||
| // Assert | ||
| Assert.IsFalse(stream.CanWrite); | ||
| } |
There was a problem hiding this comment.
Should this include Assert.Throws<NotSupportedException>(() => stream.WriteByte(1)); for consistency with the other test, and to assert that the stream is indeed not writeable?
jpreese
added
the
state: waiting for feedback
|
@Shtong I know this PR is pretty old, but in case you still want to work in this, please update your branch and address the review comments. If you don’t want to work on this someone else might be able to help get this in :) |
|
I'll try to see if I can back into that during the week-end |
|
All done. Note: the changes I made revealed a bug in the |
fgreinacher
left a comment
fgreinacher
left a comment
There was a problem hiding this comment.
Some minor things from my side, but generally this looks good.
| canWrite = streamType != StreamType.READ; | ||
| } | ||
|
|
||
| private bool canWrite = true; |
There was a problem hiding this comment.
Please move this field to the top, where the other fields are.
|
|
||
| private bool canWrite = true; | ||
|
|
||
| public override bool CanWrite |
There was a problem hiding this comment.
public override bool CanWrite => canWrite ?
| } | ||
|
|
||
| [Test] | ||
| public void MockFile_OpenRead_ShouldNotReturnWritableStream() |
There was a problem hiding this comment.
...ShouldReturnReadOnlyStream?
|
|
||
| Assert.Throws<DirectoryNotFoundException>(() => fileSystem.File.OpenWrite(filePath)); | ||
| } | ||
|
|
There was a problem hiding this comment.
Remove superfluous empty line
|
Great - thanks! |
|
Done, sorry I made a rebase to resolve merge conflicts which confused github to locate your comments! |
4 participants
Fixes #230