8000 Basic manifest implementation by sdboyer · Pull Request #4 · golang/dep · GitHub
[go: up one dir, main page]

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

Basic manifest implementation - #4

Merged
adg merged 7 commits into
golang:masterfrom
sdboyer:manifest-and-lock
Oct 18, 2016
Merged

adg merged 7 commits into
golang:masterfrom
sdboyer:manifest-and-lock

Conversation

@sdboyer
@sdboyer sdboyer commented Oct 18, 2016
Copy link
Copy Markdown
Member

Can merge this now (or discuss), and I'm now continuing on to the lock

@sdboyer
sdboyer commented Oct 18, 2016
Copy link
Copy Markdown
Member Author

Oh and yes, it's sloppy. I also don't like packages named types - super happy to go with another suggestion. Just did that for expediency - separate package seems worth it on the expectation that the manifest/lock file implementations are something that other people are likely going to want to reuse

Comment thread types/manifest.go Outdated
@@ -0,0 +1,113 @@
package types
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Add a copyright header as per the other files

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

👍

Comment thread types/manifest.go Outdated
}

func (m *Manifest) TestDependencyConstraints() gps.ProjectConstraints {
// We're not dealing with this (yet?)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

TODO: deal with this

Comment thread types/manifest_test.go Outdated
@@ -0,0 +1,94 @@
package types
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

copyright

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

👍

Comment thread types/manifest.go
}

func toProps(n string, p possibleProps) (pp gps.ProjectProperties, err error) {
if p.Branch != "" {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I like to use switches for these kinds of if/else constructs, but up to you

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

yeah, i was on the fence. i went with this because the successively narrowing checks fit better in my mind with ifs - i always have to quietly remind myself that switches DO evaluate the branches in declaration order

Comment thread types/manifest_test.go Outdated

m2, err := ReadManifest(strings.NewReader(jg))
if err != nil {
t.Errorf("Should have read Manifest correctly, but got err %q", err)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Fatalf

@adg
adg commented Oct 18, 2016
Copy link
Copy Markdown
8000 Contributor

Oh I didn't notice it was in a separate package. Just put it all in package main for now. Thanks!

@sdboyer
sdboyer commented Oct 18, 2016
Copy link
Copy Markdown
Member Author

Yeah, I did that at first, but then split it out because, afaik, Alan's sockpuppet died on the vine, so there's no tool to extract symbols into their own package. Which means it's still a bit of a PITA to do that particular refactor later.

Is your preference for single-pkg just to keep things simpler for now, or is there a specific policy at play?

@adg
adg commented Oct 18, 2016
Copy link
Copy Markdown
Contributor

It's just a simplicity thing. Especially since we don't have a good package
name. That's a strong signal to keep things together, for now.`

On 18 October 2016 at 14:45, sam boyer notifications@github.com wrote:

Yeah, I did that at first, but then split it out because, afaik, Alan's
sockpuppet died on the vine, so there's no tool to extract symbols into
their own package. Which means it's still a bit of a PITA to do that
particular refactor later.

Is your preference for single-pkg just to keep things simpler for now, or
is there a specific policy at play?

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#4 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AIDilVfgZzxM_tmJhNoYW43EBHUjAIUnks5q1EDXgaJpZM4KZV4j
.

@sdboyer
sdboyer commented Oct 18, 2016
Copy link
Copy Markdown
Member Author

kk, fair enough

@adg
adg commented Oct 18, 2016
Copy link
Copy Markdown
Contributor

Looks good.

@adg
adg merged commit 095637b into golang:master Oct 18, 2016
@sdboyer
sdboyer deleted the manifest-and-lock branch October 18, 2016 04:07
zbintliff added a commit to zbintliff/dep that referenced this pull request Mar 3, 2017
* Basic first pass at JSON-based manifest

* Add rudimentary ReadManifest() tests

* Implement gps.RootManifest methods on Manifest

* Add copyright headers

* Fatalf on test that invalidates later checks

* Rewrite TestDependencyConstraints comment as TODO

* Consolidate types package back into main
krisnova pushed a commit to krisnova/dep that referenced this pull request Apr 21, 2017
* Basic first pass at JSON-based manifest

* Add rudimentary ReadManifest() tests

* Implement gps.RootManifest methods on Manifest

* Add copyright headers

* Fatalf on test that invalidates later checks

* Rewrite TestDependencyConstraints comment as TODO

* Consolidate types package back into main
ibrasho pushed a commit to ibrasho-forks/dep that referenced this pull request May 10, 2017
* Basic first pass at JSON-based manifest

* Add rudimentary ReadManifest() tests

* Implement gps.RootManifest methods on Manifest

* Add copyright headers

* Fatalf on test that invalidates later checks

* Rewrite TestDependencyConstraints comment as TODO

* Consolidate types package back into main
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

0