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

global -p option #542

Merged
merged 13 commits into from
Jul 11, 2015
Merged

global -p option #542

merged 13 commits into from
Jul 11, 2015

Conversation

drwebb
Copy link
Contributor

@drwebb drwebb commented Jul 8, 2015

@snoyberg Any suggestions for naming these options: --local-bin-path seems to make sense, but I'm not a fan of -p. Also note that the invocation will be a lot more natural once #519 is implemented.

@drwebb drwebb mentioned this pull request Jul 8, 2015
@snoyberg
Copy link
Contributor

snoyberg commented Jul 8, 2015

I'm -1 on the -p shorthand. I'd say --install-path would be better, but perhaps @manny-fp would have input. Frankly, even --path seemed OK, but now that it's a global option I can see making it a bit more explicit would be worthwhile.

localDir <- liftIO (getAppUserDataDirectory "local") >>= parseAbsDir
return $ localDir </> $(mkRelDir "bin")
Just userPath -> do
tryPath <- try (liftIO $ canonicalizePath userPath >>= parseAbsDir)
Copy link
Contributor

Choose a reason for hiding this comment

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

This won't work if the directory doesn't already exist. Possible solution: call createDirectoryIfMissing True userPath first. Not sure if it's worth it.

@drwebb
Copy link
Contributor Author

drwebb commented Jul 8, 2015

Summary of commit be8dbb1:

Dropped -p short option. I like --path as well but --local-bin-path is a little more explicit that we are just copying the binaries to that location. I'm still up to changing to --install-path though might imply other things than just the bin. As it it --local-bin-path is clear at the expense of looking a little unconventional.

I am opposed to employing createDirectoryIfMissing. We want the user to see something like

λ church stack → λ git userlocalbin* → ~/.local/bin/stack --local-bin-path ./binz install
Could not locate user specified directory "./binz"
IOException: ./binz: canonicalizePath: does not exist (No such file or directory)

Basically fail fast and hard on a typo, not silently doing what the user doesn't want.

The other change was to match only on IOException, which is the only thing that I think canonicalizePath can throw. Looking at the code for Path, it can throw a PathParseException which I will go ahead and cover.

@drwebb
Copy link
Contributor Author

drwebb commented Jul 8, 2015

Summary of 55a5476:
Also catching PathParseExceptions right there, never been able to trigger it (canonicalizePath seems to catch any nastyness) though.

@drwebb
Copy link
Contributor Author

drwebb commented Jul 9, 2015

@snoyberg: I talked with @manny-fp about the naming and he also liked --local-bin-path as one can already use stack path --local-bin-path. I should also make sure the new field in the stack.yaml will follow the same convention, so if this looks good to you I'll handle the merge.

@snoyberg
Copy link
Contributor

snoyberg commented Jul 9, 2015

@manny-fp Please merge when you're ready, I'm fine with this PR.

@snoyberg snoyberg added this to the 0.2.0.0 milestone Jul 9, 2015
Just userPath ->
(liftIO $ canonicalizePath userPath >>= parseAbsDir)
`catches`
[Handler (\(ex :: IOException) -> error
Copy link
Contributor

Choose a reason for hiding this comment

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

Should not be using error, instead throw a new ConfigException constructor.

stack.yaml file also also uses "local-bin-path" field to specify path
drwebb pushed a commit that referenced this pull request Jul 11, 2015
--local-bin-path option for install
@drwebb drwebb merged commit 74c20f3 into master Jul 11, 2015
@snoyberg snoyberg deleted the userlocalbin branch August 13, 2015 07:46
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.

4 participants