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

Add basic Lock implementation #5

Merged
merged 2 commits into from
Oct 18, 2016
Merged

Add basic Lock implementation #5

merged 2 commits into from
Oct 18, 2016

Conversation

sdboyer
Copy link
Member

@sdboyer sdboyer commented Oct 18, 2016

Patterned quite strongly after #4

Copy link
Contributor

@adg adg left a comment

Choose a reason for hiding this comment

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

I suppose this is okay, for now. I worry that we're baking in some assumptions that gps makes, like the input hash for lock files, but let's see where it takes us.

P: make([]gps.LockedProject, len(rl.P)),
}

for k, ld := range rl.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'd go with i instead of k, as it's an index not a key

Copy link
Member Author

Choose a reason for hiding this comment

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

mm yes, fair

@sdboyer
Copy link
Member Author

sdboyer commented Oct 18, 2016

We could drop that memo prop now (though we'd still need the InputHash() method to fulfill the gps.Lock interface, ofc), but my choice to include it initially here was deliberate:

  • It is quite decoupled from pretty much all the other data, so there's little complexity cost
  • The Solutions returned from gps.Solve() will always have that hash digest available, so it's not like there's much extra work being done
  • Some of our immediately-upcoming choices would be influenced by the use of that hashing system, so it struck me as better to have it in for easy, immediate experimentation purposes than to talk about it in the abstract
  • The patterns of use it facilitates are likely to be helpful in thinking about overall system constraints, even if that's just in deciding we don't want to use it

Are there other assumptions there that concern you?

@sdboyer sdboyer merged commit f569990 into golang:master Oct 18, 2016
sdboyer added a commit that referenced this pull request Oct 18, 2016
@sdboyer
Copy link
Member Author

sdboyer commented Oct 18, 2016

(merged, given the approval, but i'm happy to still discuss)

@adg
Copy link
Contributor

adg commented Oct 18, 2016

Yeah, that all makes sense.

Are there other assumptions there that concern you?

Not that I can articulate right now. Let's see where this leads us.

zbintliff added a commit to zbintliff/dep that referenced this pull request Mar 3, 2017
ibrasho pushed a commit to ibrasho-forks/dep that referenced this pull request May 10, 2017
zjs pushed a commit to zjs/dep that referenced this pull request Mar 28, 2018
 status: process detail template output all-at-once
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants