-
Notifications
You must be signed in to change notification settings - Fork 32
AFPP #103
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
AFPP #103
Conversation
c22d2cb
to
0cf596e
Compare
353731e
to
871d199
Compare
a7dd3b8
to
950a8e4
Compare
@Bodigrim it's still not clear to me where the types should live. If they live here, it's gonna cause problems for some packages. Any package that filepath depends on will not be able to import That currently is:
with the testsuite a bit more. If we move the types somewhere else, the instances (like |
1ee5b23
to
78ebd3c
Compare
@hasufell What if |
So far, filepath has mostly avoided having IO functions at all, except for Additionally, we cannot use |
Essentially the question is where |
0be90a2
to
213e6cd
Compare
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 is massive, great work! I skimmed through, but I'm out of my depth in filepath
-specific questions.
, toPlatformString | ||
, toPlatformStringIO | ||
, bsToPlatformString | ||
, pstr |
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.
These two names are a bit cryptic, but I'm bad with naming.
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.
I think pstr
is ok for this, because it's the name for the quasiquote, which really should be short to avoid annoyances.
bsToPlatformString
expanded would be byteStringToPlatformString
... which gives me headache :p
import System.OsString | ||
import System.OsString.Internal.Types | ||
|
||
#if defined(mingw32_HOST_OS) || defined(__MINGW32__) |
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.
Not #ifdef WINDOWS
?
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.
WINDOWS
macro is only used to generate platform-specific modules (no matter the current platform), because we want to be able to deal with with both posix and windows specific filepath on all platforms. defined(mingw32_HOST_OS) || defined(__MINGW32__)
is used to select the implementation for AbstractFilePath
and PlatformFilePath
.
x3 = idx (i + 2) | ||
x4 = idx (i + 3) | ||
idx = BS.index bs | ||
{-# INLINE [0] streamUtf8 #-} |
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.
Why do you need phase control [0]
here?
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.
I don't actually know if this makes any difference.
4357f76
to
bc24013
Compare
It is much shorter than the |
3d8d2a7
to
f66f1be
Compare
e523d1e
to
2ef5702
Compare
TODO:
filepath
or somewhere else?PosixFilePath
,WindowsFilePath
andAbstractFilePath
(do we also want a platform-dependent one forPlatformFilePath
or is that redundant?), see https://github.com/hasufell/abstract-filepath/tree/master/abstract-filepath/lib/AFP/AbstractFilePath ...these will just be ctor unpacking shimsPost-PR tasks:
unix
: Add PosixFilePath and friends support (for AFPP) unix#202Win32
: Add WindwowsString/WindowsFilePath support wrt AFPP win32#198readFile
,openFile
etc.: https://github.com/hasufell/file-ioLive haddock: