feat: add plugin command #28

pchampio · 2019-09-23T20:23:30Z

Feature run-down

hover init-plugin github.com/my-organization/plugin_name initialize the current directory (must be a flutter plugin) with go support.

The files added are:
go.mod go.sum import.go.tmpl plugin.go README.md

All of them (except go.mod|sum) are generated based on the current pubspec.yaml.
- plugin.go:
  - The golang plugin package equals pubspec.yaml/name
  - The golang plugin interface equals ToCamelCase(pubspec.yaml/name) (like Android).
  - The golang plugin import equals github.com/my-organization/plugin_name/go (hover will look for a go directory, it's now a convention)
- The import.go.tmpl file is used for 'auto' import, and will be copied (with rename) to the application go/cmd upon plugin import.
hover init will check for 'lib/main_desktop.dart' file and add it if asked.
hover init execute hover plugin list
hover plugin list list golang platform plugin available (--all to also show the one without golang support)
hover plugin get will add go platform plugin by using the import.go.tmpl of that plugin (--force to re-add plugins) (respect flutter pubspec.yaml/dev_dependencies/plugin_name/path, by adding the correct replace in the 'go.mod' file (this part is done by applying a regex on the import.go.tmpl file in order to obtain the go module name)) (--dry-run performs a trial run with no changes made.)
hover plugin clean will remove unused plugin (has been added and removed from pubspec.yaml) (--purge to remove used plugin) (try to clean the 'go.mod' file, but it's not mandatory, as it doesn't affect golang compilation) (--dry-run performs a trial run with no changes made.)
hover run will apply the same check as flutter build .. do with the automatic Running "flutter pub get" in .... This check is used to suggest go platform plugin import.
hover plugin uses the local pub cache & a hosted json mapping list to look for golang plugin

fixes: go-flutter-desktop/go-flutter#260
fixes: go-flutter-desktop/go-flutter#143

waiting for PR: go-flutter-desktop/hover#28

pchampio · 2019-09-23T20:32:20Z

The following workflow works:
flutter create --template=plugin test_hover
cd test_hover
hover init-plugin github.com/my-organization/test_hover
cd example
hover init example

hover: Available plugin for this project:
     - test_hover
         version:   0.0.1
         platforms: [android, ios, go]
         import:    [Missing] The plugin can be imported by hover.
         dev:       Plugin replaced in go.mod to path: '..'

hover: run `hover plugin get` to import the missing plugins!

hover plugin get

       plugin: [test_hover] imported

hover run

hover: Target file "lib/main_desktop.dart" not found.
hover: Let hover add the "lib/main_desktop.dart" file? [y/N]: y
       Depending on your project, you might want to tweak it.
hover: Using engine from cache
hover: Cleaning the build directory
hover: Bundling flutter app
hover: Compiling 'go-flutter' and plugins

.. Then it complains about flutter.PlatformVersion because the PR isn't merged ;)
(fix it manually)

Tada the platform version is displayed!

provokateurin · 2019-09-24T15:01:58Z

Wow this looks great!

GeertJohan · 2019-09-24T22:06:07Z

cmd/plugins.go

+}
+
+var pluginCleanCmd = &cobra.Command{
+	Use:   "clean",


clean is often used as subcommand to remove everything that was created by the parent command. In this case, a user may expect hover plugin clean to remove everything that hover plugin get added. This may be confusing. I like how Go went with tidy (go mod tidy). It implies some stuff may be removed, but only when it was not needed anymore. Maybe we can go with hover plugin tidy ?

I love this proposition!

GeertJohan · 2019-09-24T22:10:20Z

cmd/plugins.go

+}
+
+var pluginCmd = &cobra.Command{
+	Use:   "plugin",


hover plugin get, hover plugin list..

It feels a little bit like we're letting hover work on a plugin that was created with init-plugin...

A project can have multiple plugins. So should this subcommand maybe be plural?

hover plugins get, hover plugins list?

GeertJohan · 2019-09-24T22:30:13Z

cmd/plugins.go

+
+	res, getErr := client.Do(req)
+	if getErr != nil {
+		return remoteList.List, err


returning err (which is nil at this point) when getErr is not nil. Why don't just name the err err instead of getErr?

GeertJohan · 2019-09-24T22:30:26Z

cmd/plugins.go

+
+	body, readErr := ioutil.ReadAll(res.Body)
+	if readErr != nil {
+		return remoteList.List, err


Same problem here, readErr vs err

GeertJohan · 2019-09-24T22:31:29Z

cmd/plugins.go

+	remoteList := &onlineList{}
+
+	client := http.Client{
+		Timeout: time.Second * 5, // Maximum of 5 secs


This is maybe a little bit low for some people on bad connections?

GeertJohan · 2019-09-24T23:02:46Z

cmd/init-plugin.go

+				fmt.Println("hover: A file or directory named `" + buildPath + "` already exists. Cannot continue init-plugin.")
+				os.Exit(1)
+			}
+		}


err can still be not nil, currently only os.IsExist(err) scenario is handled.

Ooww man, that not fair!
you can only blame you on that one^^ 😉

GeertJohan · 2019-09-24T23:05:30Z

cmd/init-plugin.go

+		}
+
+		if _, ok := getPubSpec().Flutter["plugin"]; !ok {
+			fmt.Println("hover: The directory doesn't appear to contain a plugin package. To create a flutter plugin, use `flutter create --template=plugin`.")


Users probably still need to run hover after they create the flutter plugin, because they want to have a hover plugin.

Maybe change to: "To create a new plugin, first run flutter create --template=plugin, then run hover init-plugin."

GeertJohan · 2019-09-24T23:06:05Z

cmd/plugins.go

+
+	"github.com/go-flutter-desktop/hover/internal/fileutils"
+	"github.com/pkg/errors"
+	"github.com/spf13/cobra"


imports need sorting.

note to myself: never add newlines in import statements, it messes gofmt

GeertJohan · 2019-09-24T23:06:20Z

cmd/plugins.go

+	"github.com/spf13/cobra"
+)
+
+var standaloneImplementationListAPI = "https://github.com/raw/go-flutter-desktop/plugins/master/list.json"


Can be a const?

GeertJohan · 2019-09-24T23:09:00Z

cmd/plugins.go

+			path, err := filepath.Abs(filepath.Join(dep.path, "go"))
+			if errParse != nil {
+				err = errParse
+			}


I don't understand? Why not check err before the filepath.Abs line?

Ok I need some go advice here.

The following warning message needs to be printed if readPluginGoImport or/and filepath.Abs are returning an error.

How do I don't duplicate the fmt.Printf ... warning... message without introducing a new function/goto..
What is the idiomatic way to handle errors in this context?

pchampio · 2019-09-26T17:34:05Z

I've fucked up my rebase so hard that I had to start from scratch.
see: #33

pchampio added a commit to go-flutter-desktop/plugins that referenced this pull request Sep 23, 2019

adds import.go.tmpl files

8316827

waiting for PR: go-flutter-desktop/hover#28

pchampio mentioned this pull request Sep 23, 2019

adds import.go.tmpl files go-flutter-desktop/plugins#27

Merged

pchampio requested a review from GeertJohan September 23, 2019 20:33

pchampio mentioned this pull request Sep 23, 2019

WIP: Plugin list #7

Closed

pchampio force-pushed the feature/init-plugin branch 3 times, most recently from 1b08475 to c897ac5 Compare September 23, 2019 21:22

GeertJohan requested changes Sep 24, 2019

View reviewed changes

pchampio force-pushed the feature/init-plugin branch 4 times, most recently from 9bed8a2 to 42d1ddf Compare September 26, 2019 17:28

pchampio closed this Sep 26, 2019

pchampio force-pushed the feature/init-plugin branch from 42d1ddf to 708abff Compare September 26, 2019 17:30

feat: add plugin command #28

feat: add plugin command #28

Uh oh!

Conversation

pchampio commented Sep 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pchampio commented Sep 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

provokateurin commented Sep 24, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pchampio commented Sep 26, 2019

Uh oh!

Uh oh!

pchampio commented Sep 23, 2019 •

edited

Loading

pchampio commented Sep 23, 2019 •

edited

Loading