Skip to content
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

Merged
merged 3 commits into from
May 1, 2022
Merged

AFPP #103

merged 3 commits into from
May 1, 2022

Conversation

hasufell
Copy link
Member

@hasufell hasufell commented Mar 12, 2022

TODO:


Post-PR tasks:


Live haddock:

@hasufell hasufell added the AFPP label Mar 12, 2022
@hasufell hasufell requested a review from Bodigrim March 12, 2022 21:31
@hasufell hasufell added this to In progress in AFPP via automation Mar 12, 2022
@hasufell hasufell marked this pull request as draft March 12, 2022 21:31
@hasufell hasufell force-pushed the AFPP branch 5 times, most recently from c22d2cb to 0cf596e Compare March 19, 2022 20:34
@hasufell hasufell force-pushed the AFPP branch 6 times, most recently from 353731e to 871d199 Compare March 19, 2022 22:02
@hasufell hasufell requested a review from fendor March 19, 2022 23:43
@hasufell hasufell force-pushed the AFPP branch 3 times, most recently from a7dd3b8 to 950a8e4 Compare March 20, 2022 16:51
@hasufell
Copy link
Member Author

@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 AbstractFilePath types.

That currently is:

        , bytestring >= 0.11.3.0
        , deepseq
        , exceptions
        , text
        , template-haskell

with the testsuite a bit more.

If we move the types somewhere else, the instances (like IsString) will either have to be orphan instances, because they require System.FilePath.Data.ByteString.Short.Decode and System.FilePath.Data.ByteString.Short.Encode, or we will have to inline those functions with the types.

@hasufell hasufell force-pushed the AFPP branch 7 times, most recently from 1ee5b23 to 78ebd3c Compare March 21, 2022 18:48
@Bodigrim
Copy link
Contributor

@hasufell deepseq, exceptions and template-haskell do not care about AbtractFilePath much. text potentially does, but locale-dependent Data.Text.IO and friends are an antipattern any way. bytestring is more problematic, because it has decent set of file manipulations, but there is probably no work around this.

What if filepath itself provides functions similar to Data.Bytestring.{,Lazy.}{readFile,writeFile,appendFile} and Data.ByteString.Builder.writeFile, but with AbstractFilePath instead of FilePath? Relevant bytestring documentation could suggest using them instead.

@hasufell
Copy link
Member Author

What if filepath itself provides functions similar to Data.Bytestring.{,Lazy.}{readFile,writeFile,appendFile} and Data.ByteString.Builder.writeFile, but with AbstractFilePath instead of FilePath? Relevant bytestring documentation could suggest using them instead.

So far, filepath has mostly avoided having IO functions at all, except for getSearchPath.

Additionally, we cannot use base provided functions to open files, because they don't support AbstractFilePath. We'd have to depend on unix and Win32, which of course is not possible. They have to depend on us.

Copy link
Contributor

@Bodigrim Bodigrim left a 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.

Generate.hs Outdated Show resolved Hide resolved
, toPlatformString
, toPlatformStringIO
, bsToPlatformString
, pstr
Copy link
Contributor

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.

Copy link
Member Author

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__)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not #ifdef WINDOWS?

Copy link
Member Author

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 #-}
Copy link
Contributor

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?

Copy link
Member Author

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.

System/AbstractFilePath/Data/ByteString/Short/Decode.hs Outdated Show resolved Hide resolved
System/AbstractFilePath/Data/ByteString/Short/Decode.hs Outdated Show resolved Hide resolved
System/AbstractFilePath/Data/ByteString/Short/Internal.hs Outdated Show resolved Hide resolved
System/AbstractFilePath/Data/ByteString/Short/Word16.hs Outdated Show resolved Hide resolved
System/OsString/Internal/Types.hs Outdated Show resolved Hide resolved
@hasufell
Copy link
Member Author

hasufell commented Mar 24, 2022

openFile and friends for abstract filepath are implemented here: https://github.com/hasufell/file-io

It is much shorter than the base versions. It uses unix and Win32. Whether the implementation is really equivalent to base, I don't know. I've done some manual testing (e.g. that lazy readFile "foo" >>= writeFile "bar" does not force and that windows doesn't keep files locked). For the file contents, all functions use ByteString.

@hasufell hasufell merged commit 8366ece into haskell:master May 1, 2022
AFPP automation moved this from In progress to Done May 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
AFPP
Done
Development

Successfully merging this pull request may close these issues.

None yet

2 participants