Skip to content

Update main.go#10

Open
mathew-bowersox wants to merge 7 commits intomrosset:masterfrom
mathew-bowersox:master
Open

Update main.go#10
mathew-bowersox wants to merge 7 commits intomrosset:masterfrom
mathew-bowersox:master

Conversation

@mathew-bowersox
Copy link

Exporting your read function so it can be used when using your code as a library

@mrosset
Copy link
Owner

mrosset commented Jul 15, 2019

Thanks for this patch @mathew-bowersox . I'm assuming you are importing this in which case this patch will work fine for you. However as it stands this would break main() and the tests in main_test.go. If you could update this patch so that go test works. and main() still works. I'll merge the patch.

Additionally to do this correctly main should be moved to cmd/jfect and package code should move to pkg along with tests. Then anything can import github.com/mrosset/jflect/pkg including cmd/jflect.

main/main.go Outdated
import (
"errors"
"flag"
"github.com/mathew-bowersox/jflect"
Copy link
Owner

@mrosset mrosset Jul 16, 2019

Choose a reason for hiding this comment

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

should be github.com/mrosset/jflect/pkg

package code should go into pkg/ directory

main/main.go Outdated
@@ -0,0 +1,31 @@
// Copyright 2012 The jflect Authors. All rights reserved.
Copy link
Owner

Choose a reason for hiding this comment

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

This file should now go into cmd/jflect directory

@@ -1,4 +1,4 @@
package main
package generate
Copy link
Owner

Choose a reason for hiding this comment

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

package name should be jflect

@mrosset
Copy link
Owner

mrosset commented Jul 16, 2019

@mathew-bowersox thanks for the changes, just a couple of things to fix and then almost done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants