Added first very basic widget downloader using gist github api #38

JanStevens · 2016-11-25T18:43:26Z

Allright, this is a first very basic implementation for downloading a gist and installing it as a widget. Current feature list:

Download all the files of the gist using Github Gist API
Specify the widget dir name (to prevent overwrites)
Asks before overwriting a file (using Mix.Generator.create_file)
Elixir files are seen as jobs and placed in the jobs dir

Example usage

 mix kitto.install --widget test_widget --gist JanStevens/0209a4a80cee782e5cdbe11a1e9bc393

Concerns / Ideas

I did not try to update the package.json dependencies, what if multiple widgets specify different versions? I guess the best would be for a widget to have a README with important install instructions.
Lots of assumptions and I'm a beginner elixir programmer so please feel free to add comments and suggestions to clean it up
Comming from rails background: There are external packages (gems) that define hooks so they are included in the main project (assets). It would be easy to reuse mix dependency management so every widget can be a separate mix package? Kitto could then provide a skelleton widget structure.

coveralls · 2016-11-25T18:47:33Z

Coverage decreased (-4.8%) to 75.481% when pulling 85a72d6 on playpasshq:feature/naive-widget-puller into 3f013d8 on kittoframework:master.

davejlong · 2016-11-25T20:31:11Z

One of the things we talked about in #18 is to not have a different command for widgets and jobs, instead, if the file is an Elixir file, then treat it as a job (we can update it for hooks later). If it's an HTML or JS file, then make it a widget. This would shorten the command to just mix kitto.install ...

JanStevens · 2016-11-25T20:54:03Z

Right I can easily add it

zorbash · 2016-11-25T21:48:15Z

I think that a command like mix kitto.install should be reserved for a package manager-like thing. An alternative would be to have a different binary for the installer, something in the lines of kittoctl (see: https://github.com/operable/cogctl).

At the moment i feel fetching from gist should be explicit, like mix kitto.install --gist <gist_url> and it should handle both jobs and widgets.

zorbash · 2016-11-26T13:58:08Z

@JanStevens also make sure to run credo (mix credo) and have all checks pass.

JanStevens · 2016-11-26T14:28:11Z

I've update the original MR to reflect recent changes:

Job's are now supported (any elixir file is treated as a job)
Using Mix.Generator.create_file so we ask before overwriting a file
mix credo checks passed
the --gist is now required
namespaced in install instead of widget

…jobs from gists

coveralls · 2016-11-26T14:31:27Z

Coverage decreased (-7.0%) to 78.082% when pulling ee0ce5f on playpasshq:feature/naive-widget-puller into 6a9d66e on kittoframework:master.

zorbash · 2016-11-26T14:40:15Z

lib/mix/tasks/kitto.install.ex

+  ]
+  @shortdoc "Install a custom Widget from a Github gist"
+
+  @moduledoc """


Can you place the moduledoc at the top inside the module?

zorbash · 2016-11-26T14:40:27Z

lib/mix/tasks/kitto.install.ex

+
+  # Process the install incase we have a gist and widget name
+  # It will download the gist files and place them in the right location
+  #


zorbash · 2016-11-26T14:40:46Z

lib/mix/tasks/kitto.install.ex

+  For additional information, refer to the documentation for
+  `Mix.Tasks.Run`.
+  """
+  def run(args) do


Add a separate @doc for this function.

Recursively extract the last part of the gist and use it for the building the url

…e gist does not exist anymore

…ut we didn't specify the widget directory

coveralls · 2016-11-26T16:14:50Z

Coverage increased (+1.2%) to 86.283% when pulling a574bbf on playpasshq:feature/naive-widget-puller into 6a9d66e on kittoframework:master.

zorbash · 2016-11-26T16:40:04Z

lib/mix/tasks/kitto.install.ex

+
+  # Other files all go into the widget dir
+  defp determine_file_location(file, widget_name) do
+    Map.put(file, :path, "./widgets/#{widget_name}/")


Please use Path.join to create the path.

zorbash · 2016-11-26T16:41:14Z

lib/mix/tasks/kitto.install.ex

+
+  defp download_gist(url), do: url |> HTTPoison.get! |> process_response
+
+  defp build_gist_url(gist_url) when length(gist_url) == 1, do: '#{@github_url}#{hd(gist_url)}'


Why don't you use double a quoted string?

zorbash · 2016-11-26T16:49:38Z

lib/mix/tasks/kitto.install.ex

+
+  ## Options
+
+    * `--widget` - specifies the widget name that will be used as folder name


change folder to directory

coveralls · 2016-11-26T16:51:16Z

Coverage increased (+1.2%) to 86.283% when pulling b0ff47d on playpasshq:feature/naive-widget-puller into 6a9d66e on kittoframework:master.

zorbash · 2016-11-26T16:53:33Z

test/test_helper.exs

  def atomify_map(map) do
    for {key, value} <- map, into: %{}, do: {String.to_atom(key), value}
  end
+
+  def assert_file(file) do


All those file assertions and logic deserve their own helper module.

zorbash · 2016-11-26T16:55:46Z

test/mix/tasks/kitto.install_test.exs

+  end
+
+  test "places all the files in the correct locations" do
+    in_tmp "installes widgets and jobs", fn ->


Fix typo s/installes/installs

zorbash · 2016-11-26T16:57:11Z

test/mix/tasks/kitto.install_test.exs

+
+  test "fails when `--gist` is not provided" do
+    Mix.Tasks.Kitto.Install.run(["--widget", "numbers"])
+    assert_received {:mix_shell, :error, ["Unsupported arguments"]}


Please leave a newline above assertions.

zorbash · 2016-11-26T17:01:24Z

test/mix/tasks/kitto.install_test.exs

+  test "places all the files in the correct locations" do
+    in_tmp "installes widgets and jobs", fn ->
+      with_mock HTTPoison, [get!: mock_gist_with(200, @gist_response)] do
+        Mix.Tasks.Kitto.Install.run(["--gist", "aaaa", "--widget", "number"])


Substitute "aaaaa" with something that could be an actual argument.

zorbash · 2016-11-26T17:06:54Z

test/mix/tasks/kitto.install_test.exs

+    end
+  end
+
+  test "fails when the gist has widget but no `--widget` provided" do


Most gists will contains widgets, why should the user specify --widget for all those cases?

Widget is used for the subdirectory under widgets. I added it so people could pull different number widgets by specifying a different directory

It's ok to have that flag, but when i try to fetch a gist containing a JS file with gitlab.js i think that the intuitive thing to do is to create a widgets/gitlab/ dir unless specified otherwise using the flag.

That's indeed obvious, can we agree on the following order?

Use --widget if provided

Use filename from the JS file, note we will have to sanitize it, since it can contain capitalized letters spaces and so on

Will have to think about it a bit since this will require some changes

Agreed. Let's also not sanitize the filename, it's up to the gist maintainer to have a "package" which works.

zorbash · 2016-11-26T17:10:05Z

test/test_helper.exs

+    assert File.regular?(file), "Expected #{file} to exist, but does not"
+  end
+
+  def assert_file(file, match) do


Should we mention that this code segment is taken from: https://github.com/phoenixframework/phoenix/blob/master/installer/test/mix_helper.exs
?
What do you think?

Yes I'll add reference

zorbash · 2016-11-26T17:35:42Z

test/mix/tasks/kitto.install_test.exs

+    in_tmp "installes widgets and jobs", fn ->
+      with_mock HTTPoison, [get!: mock_gist_with(200, @gist_response)] do
+        Mix.Tasks.Kitto.Install.run(["--gist", "aaaa", "--widget", "number"])
+        assert_file "widgets/number/number_job.js", fn file ->


Change file argument to contents makes it explicit that we're performing the assertion on the file contents.

coveralls · 2016-11-27T08:39:21Z

Coverage increased (+1.5%) to 86.58% when pulling 8201c50 on playpasshq:feature/naive-widget-puller into 6a9d66e on kittoframework:master.

coveralls · 2016-11-27T08:42:09Z

Coverage increased (+1.5%) to 86.58% when pulling e4f5556 on playpasshq:feature/naive-widget-puller into 6a9d66e on kittoframework:master.

zorbash · 2016-11-28T18:26:19Z

lib/mix/tasks/kitto.install.ex

+  Installs community Widget/Job from a Github Gist
+
+      mix kitto.install --widget test_widget --gist JanStevens/0209a4a80cee782e5cdbe11a1e9bc393
+      mix kitto.install --gist 0209a4a80cee782e5cdbe11a1e9bc393


Why is the second evocation missing the owner of the gist. I expected it to also have JanStevens/0209a4a80cee782e5cdbe11a1e9bc393

Because we only need the last part of the gist url, you could even copy past the full url and it would still work

I didn't know that. I tried it and it works! 🌈

zorbash · 2016-11-28T18:26:32Z

lib/mix/tasks/kitto.install.ex

@@ -0,0 +1,100 @@
+defmodule Mix.Tasks.Kitto.Install do
+  use Mix.Task
+  @shortdoc "Install community Widget/Job from a Github Gist"


Please add newline above.

zorbash · 2016-11-28T18:35:28Z

lib/mix/tasks/kitto.install.ex

+  end
+
+  # Elixir files we place in the jobs dir
+  defp determine_file_location(%{language: "Elixir"} = file, _) do


I think that we can make the assumption that .exs files are for jobs and .ex belong to lib.

Oke I'll look into it, do we also want to namespace the ex files in lib?

No need to namespace them.

zorbash · 2016-11-29T20:49:59Z

@JanStevens I rebased and tidied up the branch a bit before merging it. Also changed it to place .exs files under jobs and .ex under lib,

Added first very basic widget downloader using gist github api

85a72d6

JanStevens force-pushed the feature/naive-widget-puller branch from 531dad1 to 85a72d6 Compare November 25, 2016 18:45

zorbash added this to the 0.3.0 milestone Nov 25, 2016

Using install instead of namespace, allow to process elixir files as …

ee0ce5f

…jobs from gists

JanStevens force-pushed the feature/naive-widget-puller branch from 9abbb11 to ee0ce5f Compare November 26, 2016 14:28

zorbash reviewed Nov 26, 2016

View reviewed changes

Jan Stevens added 9 commits November 26, 2016 16:10

Reducing file length by online the supported languages array

75ffd05

Update documentation

c0998d8

Fix credo warning

0072659

Prefer Mix.shell.error

f54570d

Allow to only specify the gist part without username

b3c38d9

Recursively extract the last part of the gist and use it for the building the url

Extract constant

ca7814d

Better support for when the downloading failed from github because th…

a6136cc

…e gist does not exist anymore

Correctly handle the case when the snippet did contain widget files b…

73c34b1

…ut we didn't specify the widget directory

Added basic tests

a574bbf

zorbash reviewed Nov 26, 2016

View reviewed changes

Jan Stevens added 2 commits November 26, 2016 17:44

Use double quotes instead of single

cb54d33

Using Path.join

b0ff47d

zorbash reviewed Nov 26, 2016

View reviewed changes

Jan Stevens added 3 commits November 26, 2016 18:03

Fix typo

786bcce

Extract file assertion helpers in their own module

5d7b401

Fix test style issues

f20510f

zorbash reviewed Nov 26, 2016

View reviewed changes

Add reference for the file assertions

059a738

zorbash reviewed Nov 26, 2016

View reviewed changes

Use the widget flag as overwrite for the widget directory

8201c50

Remove unused import

e4f5556

zorbash reviewed Nov 28, 2016

View reviewed changes

zorbash closed this in d1eb705 Nov 29, 2016


		defp download_gist(url), do: url \|> HTTPoison.get! \|> process_response

		defp build_gist_url(gist_url) when length(gist_url) == 1, do: '#{@github_url}#{hd(gist_url)}'


		## Options

		* `--widget` - specifies the widget name that will be used as folder name

Added first very basic widget downloader using gist github api #38

Added first very basic widget downloader using gist github api #38

Conversation

JanStevens commented Nov 25, 2016 • edited Loading

Example usage

Concerns / Ideas

coveralls commented Nov 25, 2016

davejlong commented Nov 25, 2016

JanStevens commented Nov 25, 2016

zorbash commented Nov 25, 2016 • edited Loading

zorbash commented Nov 26, 2016

JanStevens commented Nov 26, 2016

coveralls commented Nov 26, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Nov 26, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Nov 26, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zorbash Nov 26, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Nov 27, 2016

coveralls commented Nov 27, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zorbash commented Nov 29, 2016

JanStevens commented Nov 25, 2016 •

edited

Loading

zorbash commented Nov 25, 2016 •

edited

Loading

zorbash Nov 26, 2016 •

edited

Loading