cmd/dep: Move importers under internal/importers #1145
Conversation
carolynvs
force-pushed
the
importers-package
branch
from
a76f042
to
53960fe
Compare
|
this seems like a good idea to me. don't forget to update CODEOWNERS, too 😄 |
internal/gps/identifier.go
Outdated
| @@ -167,6 +167,33 @@ type ProjectProperties struct { | |||
| Constraint Constraint | |||
| } | |||
|
|
|||
| // GetProjectPropertiesFromVersion takes a Version and returns a proper | |||
| // ProjectProperties with Constraint value based on the provided version. | |||
| func GetProjectPropertiesFromVersion(v Version) ProjectProperties { | |||
There was a problem hiding this comment.
If we're gonna do this, it needs to mirror the NewSemverConstraint(), NewSemverConstraintIC() duality.
There was a problem hiding this comment.
Maybe this is only helpful to dep anyway. I'll think about doing as you suggested vs. moving into some other pkg under internal.
There was a problem hiding this comment.
I ended up moving it back where it was and using a slimmed down version of it in the importers.
| type Importer struct { | ||
| Logger *log.Logger | ||
| Verbose bool | ||
| Sm gps.SourceManager |
There was a problem hiding this comment.
Let's make this SourceManager instead. Need to behave ourselves when exporting 😄
| got := verboseOutput.String() | ||
| want := h.GetTestFileString(goldenFile) | ||
| if want != got { | ||
| Want := h.GetTestFileString(goldenFile) |
There was a problem hiding this comment.
unnecessary capitalization - makes me wonder if there might be a few more of these that i've missed?
There was a problem hiding this comment.
Doh! My find/replace was a bit too greedy! I'll find the rest.
| @@ -2,7 +2,7 @@ | |||
| // Use of this source code is governed by a BSD-style | |||
There was a problem hiding this comment.
is it a bit redundant to name the file godep_importer.go if we're already in a package/dir named godep?
| @@ -172,7 +172,7 @@ func (i *baseImporter) loadPackages(packages []importedPackage) ([]importedProje | |||
| // * Revision constraints are ignored. | |||
| // * Versions that don't satisfy the constraint, drop the constraint. | |||
| // * Untagged revisions ignore non-branch constraint hints. | |||
| func (i *baseImporter) importPackages(packages []importedPackage, defaultConstraintFromLock bool) (err error) { | |||
| func (i *Importer) ImportPackages(packages []ImportedPackage, DefaultConstraintFromLock bool) (err error) { | |||
There was a problem hiding this comment.
s/DefaultConstraintFromLock/defaultConstraintFromLock
| type Importer struct { | ||
| Logger *log.Logger | ||
| Verbose bool | ||
| Sm gps.SourceManager |
There was a problem hiding this comment.
I'm going to go with Sam's suggestion of SourceManager.
carolynvs
force-pushed
the
importers-package
branch
3 times, most recently
from
7c84fe3
to
00cd514
Compare
|
that appveyor failure seems like it should be ephemeral, but i can't force it to retest until the merge conflicts are resolved |
carolynvs
force-pushed
the
importers-package
branch
from
00cd514
to
109f842
Compare
|
Rebased. Appveyor is now appeased. 😀 |
carolynvs
force-pushed
the
importers-package
branch
from
109f842
to
9469f9e
Compare
What does this do / why do we need it?
As the set of importers grow, it's really cluttering up the cmd/dep pkg. Here's the new structure:
What should your reviewer look out for in this PR?
I made a test package for the importer tests, so that they don't all have to be grouped into a single package. It really helped simplify the names. I don't think it will end up in the compiled dep binary because only tests reference it. Correct me if I'm wrong though!
Do you need help or clarification on anything?
Nope
Which issue(s) does this PR fix?
N/A
I think all the file based tests for the importers can be consolidated into a single test (like I did with the convert testcases) but I'll try that out in a separate PR after this is merged.