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

Implement #3264 adding --cwd to exec #3497

Merged
merged 2 commits into from
Oct 18, 2017

Conversation

khanage
Copy link
Contributor

@khanage khanage commented Oct 17, 2017

Implements feature requested by #3264 - tested it locally with stack exec.

Welcome for any suggestions as to better methods to use/docs to update.

Welcome for any suggestions as to better methods to use/docs to update.
Copy link
Contributor

@mgsloan mgsloan left a comment

Choose a reason for hiding this comment

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

Thanks! Looks pretty good, just a few things to adjust. If you don't feel like it, I can just merge this and do the adjustments myself. It's all good either way.

src/main/Main.hs Outdated
@@ -788,6 +789,20 @@ execCmd ExecOpts {..} go@GlobalOpts{..} =
pkgopts <- getPkgOpts menv wc pkgs
return (prefix ++ compilerExeName wc, pkgopts ++ args)

runWithPath path callback = case path of
Nothing -> callback
Just p | not (isValid p) -> callback
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, it seems like this would silently ignore --cwd for invalid paths. Is there a reason for this? Seems to me like it should throw an error.

src/main/Main.hs Outdated
Nothing -> callback
Just p | not (isValid p) -> callback
Just p ->
if isRelative p
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it'd be cleaner to just use System.Directory.withCurrentDirectory. I wish path had RelOrAbs or something, but for now FilePath is the best way to represent having either.


eoCwdParser :: Parser (Maybe FilePath)
eoCwdParser = optional
(strOption (long "cwd"
Copy link
Contributor

Choose a reason for hiding this comment

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

Please also add

               metavar "DIR" <>
               completer dirCompleter <>

For better help output and path completion for this option.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done :)

* Use System.Directory.withCurrentDirectory
* Throw an exception for invalid paths
* Add in better options
@mgsloan
Copy link
Contributor

mgsloan commented Oct 18, 2017

Looks great, thanks!

@mgsloan mgsloan merged commit 986bca5 into commercialhaskell:master Oct 18, 2017
@khanage khanage deleted the 3264-add-cwd-to-exec branch October 19, 2017 06:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants