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

ensure-add/avoid panic when adding current project as its own dependency #1147

Merged
merged 4 commits into from
Sep 12, 2017

Conversation

vedhavyas
Copy link
Contributor

What does this do / why do we need it?

This PR will return an error instead of panicking when current project is being added to its own dependency

What should your reviewer look out for in this PR?

This is a simple check that ensures the current project is not added as its own dependency and bail out early

Do you need help or clarification on anything?

For harness tests, I have thought of two scenarios

  1. When dep is already initialized and user tried to add the root projects as its own dependency
  2. When dep is not initialized yet

Which issue(s) does this PR fix?

fixes #1123

@jmank88
Copy link
Collaborator

jmank88 commented Sep 10, 2017

Close/open to trigger Appveyor

@jmank88 jmank88 closed this Sep 10, 2017
@jmank88 jmank88 reopened this Sep 10, 2017
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.

LGTM

@@ -500,6 +500,11 @@ func (cmd *ensureCommand) runAdd(ctx *dep.Ctx, args []string, p *dep.Project, sm
return err
}

// check if the the parsed path is the current root path
if p.ImportRoot == pc.Ident.ProjectRoot {
Copy link
Collaborator

Choose a reason for hiding this comment

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

@sdboyer We don't need strings.EqualFold or similar here, do we?

@sdboyer
Copy link
Member

sdboyer commented Sep 11, 2017 via email

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.

let's make that strings.EqualFold() change, then this is ready to merge!

Copy link
Collaborator

@ibrasho ibrasho left a comment

Choose a reason for hiding this comment

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

LGTM

@ibrasho ibrasho merged commit 16cfaf9 into golang:master Sep 12, 2017
@vedhavyas vedhavyas deleted the fix/avoid_panic branch September 12, 2017 18:05
zknill pushed a commit to zknill/dep that referenced this pull request Oct 6, 2017
…ncy (golang#1147)

* add failing test for self add

* check if the import project is current project and bail out

* add init test case

* use strings.EqualFold for path check
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

panic: Should never need to recheck imports/constraints from root during solve
5 participants