dep: Add Manifest constructor #1019
Conversation
|
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 |
ibrasho
force-pushed
the
add-Manifest-constructor
branch
from
d274487
to
0a4f68a
Compare
|
CLAs look good, thanks! |
ibrasho
force-pushed
the
add-Manifest-constructor
branch
2 times, most recently
from
ef1909c
to
5b18089
Compare
|
huh. that failure is weird. i can't see how it would have anything to do with the change here... |
Gopkg.toml
Outdated
| @@ -18,3 +18,8 @@ | |||
| [[constraint]] | |||
| name = "github.com/pkg/errors" | |||
| version = "0.8.0" | |||
|
|
|||
| [prune] | |||
There was a problem hiding this comment.
let's not actually add anything to our Gopkg.toml until the implementation is there. lead by example, and all that 😄
There was a problem hiding this comment.
Oops.This was by mistake. 😓
ibrasho
force-pushed
the
add-Manifest-constructor
branch
2 times, most recently
from
b8dd084
to
532c0f2
Compare
This commit adds a constructor for dep.Manifest. Signed-off-by: Ibrahim AshShohail <ibra.sho@gmail.com>
ibrasho
force-pushed
the
add-Manifest-constructor
branch
from
532c0f2
to
f88c233
Compare
|
I've removed extra additions that crept here by mistake. They were mainly related to adding prune options to the toml file. |
| Ovr: make(gps.ProjectConstraints), | ||
| } | ||
| g.origM = dep.NewManifest() | ||
| g.origM.Constraints = g.pd.constraints |
There was a problem hiding this comment.
The cases which overwrite the Constraints map in this way now have an extra initialization in the constructor. Multiple constructors or multiple optional paramaters seem like messy options. What about something like an Init() (or SetDefaults etc.) method instead, to be called after struct initialization, which only inits nil maps?
There was a problem hiding this comment.
Hmmm... I thought about solutions to this but couldn't come up with anything I'm comfortable with.
Manifests are initialized once or twice per dep command, so it shouldn't have any impact on performance. Right?
There was a problem hiding this comment.
Won't it be O(N+1) manifests for N imported projects (and self)?
It's still not a big deal (at least not for now). My WIP cache branch messes with these a bit (since locks and manifests are serialized/deserialized), so if that worsens the situation then I can revisit it there.
There was a problem hiding this comment.
I always forget about importers and recursively loading dependencies.
👍
|
Are y'all okay with me merging this as-is? I'd really like to have people using the new constructor in their importer PRs. |
What does this do / why do we need it?
The constructor will be used to ensure the prune variable is always set to prune nested
vendor/dirs.What should your reviewer look out for in this PR?
Did I replace all initializations of
dep.Manifest?Which issue(s) does this PR fix?
Breaking down #952