-
Notifications
You must be signed in to change notification settings - Fork 61
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
Add optional ghc-path field in bios cradles #231
Conversation
Cradle | ||
{ cradleRootDir = wdir | ||
, cradleOptsProg = CradleAction | ||
{ actionName = Types.Bios | ||
, runCradle = biosAction wdir biosCall biosDepsCall | ||
, runGhcCmd = runGhcCmdOnPath wdir | ||
, runGhcCmd = \args -> readProcessWithCwd wdir (fromMaybe "ghc" mbGhc) args "" |
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.
Would it make sense to canonicalize the path if it is relative?
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.
Good question, I'm not sure. The path is only ever used in this call to readProcess
, so I think it probably doesn't matter much.
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.
Only one question, otherwise LGTM
Co-authored-by: fendor <fendor@users.noreply.github.com>
Thanks for the quick review! Once I fix the test failures, I'm very keen on a minor release incorporating this change as it will allow us to start using ghcide in some parts of our Haskelll codebase at work. Thanks again! |
The travis tests fail because you need to add the test files to the |
Fixes #215 to a simpler extent, in that the
with-ghc
field is of typeString
rather thanIO String
.