-
Notifications
You must be signed in to change notification settings - Fork 4
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
Support deep export via optional '--deep' argument #6
Conversation
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.
Overall this looks awesome! Can you also update the README?
source/library/Autoexporter.hs
Outdated
import qualified Distribution.ModuleName as ModuleName | ||
import qualified Distribution.Text as Text | ||
import qualified System.Directory as Directory | ||
import qualified System.Environment as Environment | ||
import qualified System.FilePath as FilePath | ||
|
||
data ExportScope = ExportScope'Shallow | ||
| ExportScope'Deep |
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 haven't seen this style of constructor names before. Can you remove the apostrophes (ExportScopeShallow
)?
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.
Will do.
source/library/Autoexporter.hs
Outdated
pure $ mconcat (filter isHaskellFile rootItems : childItems) | ||
|
||
findRootFiles :: FilePath -> IO [FilePath] | ||
findRootFiles dir = filter isRootItem <$> Directory.getDirectoryContents dir |
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.
Can you use listDirectory
instead? It is like getDirectoryContents
except that it avoids .
and ..
.
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.
Sweet - didn't know about this function!
source/library/Autoexporter.hs
Outdated
let output = makeOutput moduleName directory files | ||
writeFile outputFile output | ||
|
||
findFiles :: ExportScope -> FilePath -> IO [FilePath] | ||
findFiles ExportScope'Shallow dir = filter isHaskellFile <$> findRootFiles dir | ||
findFiles ExportScope'Deep dir = do |
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.
Can you change this to a single top-level declaration with a case
expression? I don't like duplicating function declarations.
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.
Np.
source/library/Autoexporter.hs
Outdated
let output = makeOutput moduleName directory files | ||
writeFile outputFile output | ||
|
||
findFiles :: ExportScope -> FilePath -> IO [FilePath] | ||
findFiles ExportScope'Shallow dir = filter isHaskellFile <$> findRootFiles dir |
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.
Can you refactor this to avoid using <$>
? I know it's a pretty popular operator, but I try to avoid using it. Either fmap
or do
notation is fine.
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.
Will do.
source/library/Autoexporter.hs
Outdated
let path = dir FilePath.</> item | ||
dirExists <- Directory.doesDirectoryExist path | ||
if dirExists | ||
then fmap (item FilePath.</>) <$> findFiles ExportScope'Deep path |
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.
Same deal here with <$>
.
Also: Maybe the "deep" branch of this function should be a separate helper function? That way this recursive call doesn't have to provide the export scope.
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.
Done and done.
Addressed PR comments and updated README. |
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.
Sorry for taking so long to review this. Looks great!
This closes #5.
The new behavior is optional. The standard way to use
autoexporter
is via{-# OPTIONS_GHC -F -pgmF autoexporter #-}
and this does shallow re-exporting.
If a user wants deep re-exporting, they can do
{-# OPTIONS_GHC -F -pgmF autoexporter -optF --deep #-}
.