Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

lib-version implementation #668

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions cmd/fyne/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ func loadCommands() {
commands["get"] = &getter{}
commands["package"] = &packager{}
commands["install"] = &installer{}
commands["lib-version"] = &versioner{}
}

func main() {
Expand Down
74 changes: 74 additions & 0 deletions cmd/fyne/version.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,74 @@
package main

import (
"fmt"
"go/build"
"os"
"strings"

"github.com/pkg/errors"
)

const pkg = "fyne.io/fyne"

// Declare conformity to command interface
var _ command = (*versioner)(nil)

type versioner struct {
context build.Context
}

func (v *versioner) run(_ []string) {
err := v.validate()
if err != nil {
fmt.Fprintln(os.Stderr, err.Error())
os.Exit(1)
}

err = v.doVersion()
if err != nil {
fmt.Fprintf(os.Stderr, "%s\n", err.Error())
os.Exit(1)
}
}

func (v *versioner) validate() error {
xxxserxxx marked this conversation as resolved.
Show resolved Hide resolved
return nil
}

func (v *versioner) doVersion() error {
c := build.Default
p, e := c.Import(pkg, ".", build.FindOnly)
Copy link
Member

Choose a reason for hiding this comment

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

c, p, e and ve in this method are confusing - what are they short for?
Go normally uses "err" for an error var so that's a start. Could the other variables be more clearly named?

Copy link
Member

Choose a reason for hiding this comment

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

Perhaps in this instance build.Default could be set to "b" but I don't follow "c" as a choice. If "e" were "err" I still don't know what "p" and "ve" indicate?

if e != nil {
return e
}
ve, e := parsePkgString(p.PkgObj)
if e != nil {
return e
}
fmt.Println(ve)
return nil
}

func parsePkgString(s string) (string, error) {
// @ separates the package path from the version
ps := strings.Split(s, "@")
if len(ps) < 2 {
return "", errors.New(fmt.Sprintf("No version information in %s; not using modules?", s))
}
// PkgObj contains some junk after the version -- "junk," in this case,
// meaning some internal information I don't understand. It's not part of
// the version, in any case.
vs := strings.Split(ps[1], "/")
if len(vs) < 1 {
return "", errors.New(fmt.Sprintf("Missing version information %s; not using modules?", s))
}
return vs[0], nil
}

func (v *versioner) printHelp(indent string) {
fmt.Println(indent, "Print the version of the Fyne library that would be used to build the package.")
}

func (v *versioner) addFlags() {
}
32 changes: 32 additions & 0 deletions cmd/fyne/version_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
package main

import (
"testing"

"github.com/stretchr/testify/assert"
)

func TestParsePkgString(t *testing.T) {
type test struct {
in string // Test input
es string // Expected output
Copy link
Member

Choose a reason for hiding this comment

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

Can you use meaningful variable names? If it was "expectedOutput" then you wouldn't need the comment...

Copy link
Author

Choose a reason for hiding this comment

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

So, the Fyne codebase doesn't follow Go stdlib conventions?

Copy link
Member

Choose a reason for hiding this comment

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

I would not go so far as to say Fyne does not follow Go conventions, but I would say that the project follows the belief that if you have to have inline comments then your code could be clearer. Of course "i" is an iterator and "b" could be a buffer - where the context is clear and a convention is followed. I do not think that "es" is common shorthand for "expected output" - looking at slide#8 It would seem to indicate that "expected" could be completely inline with their recommendation.

ee bool // Expecting error?
Copy link
Member

Choose a reason for hiding this comment

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

expectedError would imply an error type, perhaps shouldError is closer to the truth for a bool?

}

ts := []test{
{in: "/home/user/go/pkg/mod/fyne.io/fyne@v1.2.2/pkg/li", es: "v1.2.2", ee: false},
{in: "fyne.io/fyne@v1.1", es: "v1.1", ee: false},
{in: "/home/user/fyne/pkg/linux_amd64/fyne.io/fyne.a", es: "", ee: true},
{in: "v1.2.2", es: "", ee: true},
{in: "/home/user/go/pkg/mod/github.com/peterbourgon/diskv@v2.0.1+incompatible/pkg/linux_amd64/github.com/peterbourgon/diskv.a", es: "v2.0.1+incompatible", ee: false},
{in: "/home/ser/go/pkg/mod/github.com/peterbourgon/diskv/v3@v3.0.0/pkg/linux_amd64/github.com/peterbourgon/diskv/v3.a", es: "v3.0.0", ee: false},
}

for _, tc := range ts {
gs, ge := parsePkgString(tc.in)
Copy link
Member

Choose a reason for hiding this comment

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

It's not clear from the context what gs and ge are - could they be named more clearly?

assert.Equal(t, tc.es, gs)
if tc.ee {
assert.Error(t, ge)
}
}
}