Port cmd/fyne to using urfave/cli #1858

Jacalz · 2021-01-24T17:25:16Z

Description:

This PR moves ahead with the task of porting over the entire cmd/fyne cli application to using https://github.com/urfave/cli. It currently needs more testing and I would like if people take a look at my proposed flag aliases and names along with any usages or descriptions that need to be updated with better wording. Of note is that the cmd/fyne/commands package has a couple breakages, but it should only affect the commands themselves (I don't think the intention ever was to let other people create cli applications with it).

The work on porting to cobra was abandoned due to the fact that flags had to be posix compliant.
Supersedes #1312

Checklist:

Tests included.
Lint and formatter run with no errors.
Tests all pass.

Where applicable:

Public APIs match existing style.
Any breaking changes have a deprecation path or have been discussed.

AlbinoGeek · 2021-01-25T00:15:22Z

I really don't like single dash command arguments -key=val that Golang does, they specifically chose not to listen to the standardized way of doing things, with no discernible benefit whatsoever (except typing one less dash? but arguments are auto-completed by any shell written in the last 10 years... so who cares?)

In fact, it makes single dash arguments "not chainable", which is pretty dumb. (see: any *nix command ever: cmd -ancRQ)

Which brings me to the point of that rant:

Does urfive support auto-complete? fyne r<tab>elease etc
Does urfive support levenshtein distance-based auto-correction? fyne rlesae -> fyne release

Those were the only real real features that cobra provided over any other cli impl. (Well, that and viper integration.)

Alright, review done, I'm sorry, haha.

cmd/fyne/commands/deprecated.go

cmd/fyne/commands/get.go

cmd/fyne/commands/install.go

AlbinoGeek · 2021-01-25T00:38:09Z

cmd/fyne/commands/release.go

+			&cli.StringFlag{
+				Name:        "target",
+				Aliases:     []string{"os"},
+				Usage:       "The operating system to target (android, android/arm, android/arm64, android/amd64, android/386, darwin, freebsd, ios, linux, netbsd, openbsd, windows)",


What is android v.s android/386 ? Is that practically android/arm386?

Also, where did 32-bit BSD, Linux and Windows go?

We probably shouldn't list all of possible values in the usage. The list we have is way too long.

Makes sense. If extended helps comes up somewhere, we could link an FAQ or developer.fyne page that has information on building, or lists architectures.

cmd/fyne/commands/release.go

cmd/fyne/main.go

go.mod

andydotxyz · 2021-01-26T20:14:38Z

Is there any way we can test that this change is backward compatible?
I like the idea, as discussed on the call, but it's important that the changes are all aliased/deprecated to not break any usage.

cmd/fyne/commands/get.go

AlbinoGeek

Love what I'm seeing :) New changes look great.

Also good to know I'm now the only one using spew.Dump() to debug :P

cmd/fyne/commands/deprecated.go

Jacalz · 2021-01-30T16:50:59Z

Love what I'm seeing :) New changes look great.

Also good to know I'm now the only one using spew.Dump() to debug :P

Thanks. I don't use spew.Dump(). Where did you get that from?

AlbinoGeek · 2021-01-30T23:16:31Z

Love what I'm seeing :) New changes look great.
Also good to know I'm now the only one using spew.Dump() to debug :P

Thanks. I don't use spew.Dump(). Where did you get that from?

I got that from the go.sum removal of a lingering import of github.com/davecgh/go-spew.

andydotxyz

I think a few changes like re-ordering and doc removal need to be reversed.

The main problem here is that we have a new public API that is exposing 3rd party CLI interface types instead of our own. This is something we try to avoid, the solution may need some more thinking.

cmd/fyne/commands/bundle.go

andydotxyz

Overall this is looking good - sorry it has been so much work!

There is a problem - as far as I can see NewGetter has been removed from the public API.
The struct is there, but not the constructor or methods.

cmd/fyne/internal/commands/deprecated.go

andydotxyz

So close.

However when I try to use this the Get(string) and SetIcon(string) methods are not found on Getter. We may need public definitions that wrap...

Or you could use type Getter = commands.Getter to expose all of the internally-public methods.

andydotxyz

Looks good to me. Might be good to get another 👍 if possible, but it won't block merging.

go.sum

AlbinoGeek

Looks like everything I'd commented on has been addressed.

Jacalz · 2021-02-06T13:00:53Z

Sorry for the force push. Had to rebase on develop and git wouldn't let me push normally.

WIll need work in the future to propertly expose bundler, packager, installer and releaser.

andydotxyz

Looks like a random code addition snuck into cmd/hello/main.go

cmd/hello/main.go

Jacalz changed the title ~~Urfave cli~~ Port cmd/fyne to using urfave/cli Jan 24, 2021

AlbinoGeek reviewed Jan 25, 2021

View reviewed changes

andydotxyz requested changes Jan 26, 2021

View reviewed changes

cmd/fyne/commands/get.go Show resolved Hide resolved

AlbinoGeek reviewed Jan 28, 2021

View reviewed changes

cmd/fyne/commands/deprecated.go Outdated Show resolved Hide resolved

Jacalz marked this pull request as ready for review January 30, 2021 18:10

Jacalz mentioned this pull request Jan 30, 2021

Add support for building on DragonflyBSD #1885

Closed

3 tasks

andydotxyz requested changes Feb 1, 2021

View reviewed changes

cmd/fyne/commands/bundle.go Show resolved Hide resolved

cmd/fyne/commands/bundle.go Show resolved Hide resolved

cmd/fyne/commands/bundle.go Outdated Show resolved Hide resolved

cmd/fyne/commands/bundle.go Outdated Show resolved Hide resolved

andydotxyz requested changes Feb 4, 2021

View reviewed changes

cmd/fyne/internal/commands/deprecated.go Outdated Show resolved Hide resolved

Jacalz requested a review from andydotxyz February 5, 2021 18:46

andydotxyz requested changes Feb 6, 2021

View reviewed changes

andydotxyz previously approved these changes Feb 6, 2021

View reviewed changes

AlbinoGeek reviewed Feb 6, 2021

View reviewed changes

go.sum Outdated Show resolved Hide resolved

AlbinoGeek previously approved these changes Feb 6, 2021

View reviewed changes

Jacalz dismissed stale reviews from AlbinoGeek and andydotxyz via 9eb0f4b February 6, 2021 13:00

Jacalz force-pushed the urfave-cli branch from f3f691b to 9eb0f4b Compare February 6, 2021 13:00

Jacalz added 9 commits February 6, 2021 14:03

Initial version of cmd/fyne ported to urfave/cli

1d19919

Fix duplicated flag for bundle command

f25ad26

Add note on deprecated commands

fb6cceb

A few usage text fixes

c9a858e

Fix various issues

8c80fda

Make staticcheck happy

f329d70

Fix a missing return

35e9812

Add in all deprecated stuff

6908f7d

Major rearchitecture to not export cli commands

2532ce7

WIll need work in the future to propertly expose bundler, packager, installer and releaser.

Jacalz added 7 commits February 6, 2021 14:03

Fix a few issues from the rearchitecture

2271ac7

Better deprecation text

d456120

Add back the NewGetter commands, sorry

bac5a24

Move back to the version handling that we had before

f02ca7d

Have --version and -v enabled as well (posix)

2b2c9eb

Fix the alias of the getter

4f12a76

Remove some strange stuff from go.sum

9eb0f4b

andydotxyz requested changes Feb 6, 2021

View reviewed changes

cmd/hello/main.go Outdated Show resolved Hide resolved

Remove a piece of code that slipped in

9307321

andydotxyz approved these changes Feb 6, 2021

View reviewed changes

Jacalz merged commit 75ddc86 into fyne-io:develop Feb 6, 2021

Jacalz deleted the urfave-cli branch February 6, 2021 19:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Port cmd/fyne to using urfave/cli #1858

Port cmd/fyne to using urfave/cli #1858

Jacalz commented Jan 24, 2021 •

edited

Loading

AlbinoGeek commented Jan 25, 2021 •

edited

Loading

AlbinoGeek Jan 25, 2021

Jacalz Jan 28, 2021

AlbinoGeek Jan 29, 2021

andydotxyz commented Jan 26, 2021

AlbinoGeek left a comment

Jacalz commented Jan 30, 2021

AlbinoGeek commented Jan 30, 2021 •

edited

Loading

andydotxyz left a comment

andydotxyz left a comment

andydotxyz left a comment •

edited

Loading

andydotxyz left a comment

AlbinoGeek left a comment

Jacalz commented Feb 6, 2021

andydotxyz left a comment

Port cmd/fyne to using urfave/cli #1858

Port cmd/fyne to using urfave/cli #1858

Conversation

Jacalz commented Jan 24, 2021 • edited Loading

Description:

Checklist:

Where applicable:

AlbinoGeek commented Jan 25, 2021 • edited Loading

AlbinoGeek Jan 25, 2021

Choose a reason for hiding this comment

Jacalz Jan 28, 2021

Choose a reason for hiding this comment

AlbinoGeek Jan 29, 2021

Choose a reason for hiding this comment

andydotxyz commented Jan 26, 2021

AlbinoGeek left a comment

Choose a reason for hiding this comment

Jacalz commented Jan 30, 2021

AlbinoGeek commented Jan 30, 2021 • edited Loading

andydotxyz left a comment

Choose a reason for hiding this comment

andydotxyz left a comment

Choose a reason for hiding this comment

andydotxyz left a comment • edited Loading

Choose a reason for hiding this comment

andydotxyz left a comment

Choose a reason for hiding this comment

AlbinoGeek left a comment

Choose a reason for hiding this comment

Jacalz commented Feb 6, 2021

andydotxyz left a comment

Choose a reason for hiding this comment

Jacalz commented Jan 24, 2021 •

edited

Loading

AlbinoGeek commented Jan 25, 2021 •

edited

Loading

AlbinoGeek commented Jan 30, 2021 •

edited

Loading

andydotxyz left a comment •

edited

Loading