Skip to content
This repository was archived by the owner on Sep 9, 2020. It is now read-only.

Conversation

niranjan92
Copy link
Contributor

@niranjan92 niranjan92 commented May 10, 2017

Fixes #537

*New to golang and this is my first Pull Request.

Copy link
Member

@sdboyer sdboyer left a comment

Choose a reason for hiding this comment

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

Wooooo, awesome, welcome!!! 🎉 🎉 You are not the first person new to go to be making PRs against dep - you're in good company 😄

cmd/dep/init.go Outdated
func createDirIfNotExist(path string) error {
if _, err := os.Stat(path); err != nil {
if os.IsNotExist(err) {
if e := os.Mkdir(path, os.FileMode(0755)); e != nil {
Copy link
Member

Choose a reason for hiding this comment

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

os.MkdirAll here will take care of creating any intermediary directories, if necessary.

Also, let's use 0777 so that it's entirely up to the umask to decide what perms are applied.

cmd/dep/init.go Outdated
@@ -68,6 +81,12 @@ func (cmd *initCommand) Run(ctx *dep.Ctx, loggers *Loggers, args []string) error
root = ctx.WorkingDir
} else {
root = args[0]
if !filepath.IsAbs(args[0]) {
Copy link
Member

Choose a reason for hiding this comment

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

I think what we really want here is filepath.Clean(). That should do all the work you're manually doing here, plus a little more.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i checked filepath.Clean(), it doesn't generate absolute path and dep init fails later.
Eg. dep init new_test_project would fail as filepath.clean would return new_test_project as the output.

Let me know if I am missing something.

@sdboyer
Copy link
Member

sdboyer commented May 11, 2017

Oh, forgot to mention in my previous review - we'll also want some tests to cover this.

@niranjan92
Copy link
Contributor Author

will add tests today.

@googlebot
Copy link
Collaborator

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that they're okay with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants