-
Notifications
You must be signed in to change notification settings - Fork 1k
Conversation
Oh and yes, it's sloppy. I also don't like packages named |
@@ -0,0 +1,113 @@ | |||
package types |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
} | ||
|
||
func (m *Manifest) TestDependencyConstraints() gps.ProjectConstraints { | ||
// We're not dealing with this (yet?) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO: deal with this
@@ -0,0 +1,94 @@ | |||
package types |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
copyright
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
} | ||
|
||
func toProps(n string, p possibleProps) (pp gps.ProjectProperties, err error) { | ||
if p.Branch != "" { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
|
||
m2, err := ReadManifest(strings.NewReader(jg)) | ||
if err != nil { | ||
t.Errorf("Should have read Manifest correctly, but got err %q", err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fatalf
Oh I didn't notice it was in a separate package. Just put it all in package main for now. Thanks! |
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? |
It's just a simplicity thing. Especially since we don't have a good package On 18 October 2016 at 14:45, sam boyer notifications@github.com wrote:
|
kk, fair enough |
Looks good. |
* 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
* 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
* 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
Can merge this now (or discuss), and I'm now continuing on to the lock