-
Notifications
You must be signed in to change notification settings - Fork 2
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
Adding 'MustExist' constructor to assert a file is assumed to exist #85
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -85,8 +85,11 @@ data AllowExisting | |
-- ^ The file may already exist. If it does, it is reopened. If it | ||
-- doesn't, it is created. | ||
| MustBeNew | ||
-- ^ The file may not yet exist. If it does, an error | ||
-- ^ The file must not yet exist. If it does, an error | ||
-- ('FsResourceAlreadyExist') is thrown. | ||
| MustExist | ||
-- ^ The file must already exist. If it does not, an error | ||
-- ('FsResourceDoesNotExist') is thrown. | ||
deriving (Eq, Show) | ||
|
||
allowExisting :: OpenMode -> AllowExisting | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wonder, should Let's also take this opportunity to document the semantics of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We could also add a TODO to test that this is the actual behaviour on both Windows and Unix. Hopefully this is already checked by the state machine tests, but it does not hurt to have some more targeted property tests There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think there should probably also be an issue to properly document |
||
|
@@ -460,6 +463,7 @@ instance Condense SeekMode where | |
instance Condense AllowExisting where | ||
condense AllowExisting = "" | ||
condense MustBeNew = "!" | ||
condense MustExist = "+" | ||
|
||
instance Condense OpenMode where | ||
condense ReadMode = "r" | ||
|
Original file line number | Diff line number | Diff line change | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -231,14 +231,21 @@ getDir fp = | |||||||||||
Specific file system functions | ||||||||||||
-------------------------------------------------------------------------------} | ||||||||||||
|
||||||||||||
-- | Open a file: create it if necessary or throw an error if it existed | ||||||||||||
-- already wile we were supposed to create it from scratch (when passed | ||||||||||||
-- 'MustBeNew'). | ||||||||||||
-- | Open a file: create it if necessary or throw an error if either: | ||||||||||||
-- 1. It existed already while we were supposed to create it from scratch | ||||||||||||
-- (when passed 'MustBeNew'). | ||||||||||||
-- 2. It did not already exists when we expected to (when passed 'MustExist'). | ||||||||||||
Comment on lines
+234
to
+237
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's link to
Suggested change
|
||||||||||||
openFile :: Monoid a | ||||||||||||
=> FsPath -> AllowExisting -> FsTree a -> Either FsTreeError (FsTree a) | ||||||||||||
openFile fp ex = alterFile fp Left (Right mempty) $ \a -> case ex of | ||||||||||||
AllowExisting -> Right a | ||||||||||||
MustBeNew -> Left (FsExists fp) | ||||||||||||
openFile fp ex = alterFile fp Left caseDoesNotExist caseAlreadyExist | ||||||||||||
where | ||||||||||||
caseAlreadyExist a = case ex of | ||||||||||||
MustBeNew -> Left (FsExists fp) | ||||||||||||
_ -> Right a | ||||||||||||
|
||||||||||||
caseDoesNotExist = case ex of | ||||||||||||
MustExist -> Left (FsMissing fp (pathLast fp :| [])) | ||||||||||||
_ -> Right mempty | ||||||||||||
Comment on lines
+242
to
+248
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Preferably, list all constructors, so that we won't forget to update the cases if the |
||||||||||||
|
||||||||||||
-- | Replace the contents of the specified file (which must exist) | ||||||||||||
replace :: FsPath -> a -> FsTree a -> Either FsTreeError (FsTree a) | ||||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -479,7 +479,11 @@ hOpen fp openMode = do | |
, fsLimitation = True | ||
} | ||
modifyMockFS $ \fs -> do | ||
let alreadyHasWriter = | ||
let assumedExistance (WriteMode MustExist) = True | ||
assumedExistance (AppendMode MustExist) = True | ||
assumedExistance (ReadWriteMode MustExist) = True | ||
assumedExistance _ = False | ||
Comment on lines
+482
to
+485
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe name it |
||
alreadyHasWriter = | ||
any (\hs -> openFilePath hs == fp && isWriteHandle hs) $ | ||
openHandles fs | ||
when (openMode /= ReadMode && alreadyHasWriter) $ | ||
|
@@ -491,7 +495,7 @@ hOpen fp openMode = do | |
, fsErrorStack = prettyCallStack | ||
, fsLimitation = True | ||
} | ||
when (openMode == ReadMode) $ void $ | ||
when (openMode == ReadMode || assumedExistance openMode) $ void $ | ||
checkFsTree $ FS.getFile fp (mockFiles fs) | ||
Comment on lines
+498
to
499
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we make the change |
||
files' <- checkFsTree $ FS.openFile fp ex (mockFiles fs) | ||
return $ newHandle (fs { mockFiles = files' }) | ||
|
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
|
@@ -676,7 +676,7 @@ generator Model{..} = oneof $ concat [ | |||
(rf, wf) = if fileExists then (10,3) else (1,3) | ||||
|
||||
genAllowExisting :: Gen AllowExisting | ||||
genAllowExisting = elements [AllowExisting, MustBeNew] | ||||
genAllowExisting = elements [AllowExisting, MustBeNew, MustExist] | ||||
|
||||
genSeekMode :: Gen SeekMode | ||||
genSeekMode = elements [ | ||||
|
@@ -1055,6 +1055,7 @@ data Tag = | |||
-- > Get .. | ||||
| TagPutSeekNegGet | ||||
|
||||
|
||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||
-- Open with MustBeNew (O_EXCL flag), but the file already existed. | ||||
-- | ||||
-- > h <- Open fp (AppendMode _) | ||||
|
@@ -1063,6 +1064,13 @@ data Tag = | |||
| TagExclusiveFail | ||||
|
||||
|
||||
-- Open with MustExist, but the file does not exist. | ||||
-- | ||||
-- > DoesFileExist fp | ||||
-- > h <- Open fp (AppendMode _) | ||||
| TagAssumeExists | ||||
Comment on lines
+1067
to
+1071
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A similar tag for opening a non-existent file in read mode would also be useful |
||||
|
||||
|
||||
-- Reading returns an empty bytestring when EOF | ||||
-- | ||||
-- > h <- open fp ReadMode | ||||
|
@@ -1136,6 +1144,7 @@ tag = C.classify [ | |||
, tagPutSeekGet Set.empty Set.empty | ||||
, tagPutSeekNegGet Set.empty Set.empty | ||||
, tagExclusiveFail | ||||
-- , tagAssumeExistsFail -- Set.empty | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should be updated |
||||
, tagReadEOF | ||||
, tagPread | ||||
, tagPutGetBuf Set.empty | ||||
|
@@ -1481,6 +1490,23 @@ tag = C.classify [ | |||
Left TagExclusiveFail | ||||
_otherwise -> Right tagExclusiveFail | ||||
|
||||
tagAssumeExistsFail :: EventPred | ||||
tagAssumeExistsFail = C.predicate $ \ev -> | ||||
{- | ||||
tagClosedTwice closed = successful $ \ev _suc -> | ||||
case eventMockCmd ev of | ||||
Close (Handle h _) | Set.member h closed -> Left TagClosedTwice | ||||
Close (Handle h _) -> Right $ tagClosedTwice $ Set.insert h closed | ||||
_otherwise -> Right $ tagClosedTwice closed | ||||
(DoesFileExist _, Bool False) -> Left TagDoesFileExistKO | ||||
-} | ||||
Comment on lines
+1495
to
+1502
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Remove commented out code |
||||
case (eventMockCmd ev, eventMockResp ev) of | ||||
(Open _ mode, Resp (Left fsError)) | ||||
| MustExist <- allowExisting mode | ||||
, fsErrorType fsError == FsResourceDoesNotExist -> | ||||
Left TagAssumeExists | ||||
_otherwise -> Right tagAssumeExistsFail | ||||
|
||||
tagReadEOF :: EventPred | ||||
tagReadEOF = successful $ \ev suc -> | ||||
case (eventMockCmd ev, suc) of | ||||
|
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.
This seems to be wrong:
MustExist
andAllowExisting
get the same file flags