Path refactoring #2918
Path refactoring #2918
Conversation
harendra-kumar
force-pushed
the
path-refactoring
branch
from
a2c806c to
e1e5467
Compare
harendra-kumar
force-pushed
the
path-refactoring
branch
from
e1e5467 to
5406440
Compare
| -- the places where the predicate matches in the stream. | ||
| -- | ||
| -- >>> Stream.toList $ Stream.indexEndBy_ (== '/') $ Stream.fromList "/home/harendra" |
There was a problem hiding this comment.
/home/harendra
Now the world knows where your home directory is located.
| -- | @C:...@, does not check array length. | ||
| {-# INLINE unsafeHasDrive #-} | ||
| unsafeHasDrive :: (Unbox a, Integral a) => Array a -> Bool | ||
| unsafeHasDrive a | ||
| -- Check colon first for quicker return | ||
| | unsafeIndexChar 1 a /= ':' = False |
There was a problem hiding this comment.
a /= b == False is a complex way of saying a == b
There was a problem hiding this comment.
This is function definition, not a comparison.
| -- Assuming the path is not empty. | ||
| isSeparator Windows (wordToChar (Array.getIndexUnsafe 0 a)) | ||
| | wordToChar (Array.getIndexUnsafe 0 a) /= '.' = False |
There was a problem hiding this comment.
This is unsafe. We don't check the length before indexing.
There was a problem hiding this comment.
It can be written as wordToChar (Array.getIndexUnsafe 0 a) == '.'
It's easier to understand.
There was a problem hiding this comment.
No it cannot be written like that. This is function definition.
There was a problem hiding this comment.
This is unsafe. We don't check the length before indexing.
It assumes path is not empty. This assumption is there at some places already. If it is legal to have non-empty path can you raise an issue to fix these?
| | otherwise = True | ||
| -- | The path starting with a separator. On Windows this is relative to current | ||
| -- drive while on Posix this is absolute path as there is only one drive. | ||
| isRelativeCurDrive :: (Unbox a, Integral a) => OS -> Array a -> Bool |
There was a problem hiding this comment.
Can you add some examples for this?
isRelativeCurDrive is any path that starts with a separator?
There was a problem hiding this comment.
"/home/user" is relative to current drive.
| Stream.toList | ||
| $ Stream.take 3 | ||
| $ Stream.takeWhile (not . isSeparator os) | ||
| $ fmap wordToChar |
There was a problem hiding this comment.
We are converting all words to characters.
If the input is UTF16 (Or UTF8) encoded, will all the words translate into proper characters?
We should instead use isSeperatorIntegral, which checks the underlying Word instead of converting it into a Char.
There was a problem hiding this comment.
I do not see a real problem here.
There was a problem hiding this comment.
Ah, yes. The problem might arise when we are printing it. You can ignore this comment.
| -- XXX On posix we just need to check last 3 bytes of the array | ||
| -- XXX Display the path in the exception messages. | ||
| case s1 of | ||
| [] -> throwM $ InvalidPath "A file name cannot have a trailing separator" |
There was a problem hiding this comment.
Even an empty path could result in this. The error message does not indicate that.
So, maybeFile would fail if the path ends with
...
There was a problem hiding this comment.
Empty path is not a valid path.
So, maybeFile would fail if the path ends with
yes.
No description provided.