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

Add generators #53

Closed
wants to merge 57 commits into from
Closed

Conversation

crertel
Copy link
Contributor

@crertel crertel commented Feb 3, 2021

This PR is about adding mix scenic.gen.scene and mix scenic.gen.component helpers.

It's currently a work in progress, but if this is looking like a good idea we can wrap it up.

Current questions are:

  • How to we get the proper base module name, e.g. MyApp in MyApp.Scenes.SceneName when we've run mix scenic.gen.scene scene_name?
  • Are there additional considerations around umbrella apps or whatever?

@Eiji7
Copy link

Eiji7 commented Feb 3, 2021

How to we get the proper base module name, e.g. MyApp in MyApp.Scenes.SceneName when we've run mix scenic.gen.scene scene_name?

Simply look how Phoenix core team solves it … I recommend to read especially Mix.Phoenix code.

Here you have app module name:
https://github.com/phoenixframework/phoenix/blob/e05af0ad5527c9a192de122f3e43d7ed4c7a4c9f/lib/mix/phoenix.ex#L123-L150

Here you have web path:
https://github.com/phoenixframework/phoenix/blob/e05af0ad5527c9a192de122f3e43d7ed4c7a4c9f/lib/mix/phoenix.ex#L196-L204

Are there additional considerations around umbrella apps or whatever?

https://github.com/phoenixframework/phoenix/blob/e05af0ad5527c9a192de122f3e43d7ed4c7a4c9f/lib/mix/tasks/phx.gen.schema.ex#L102-L104

Short: don't complicate and simply raise 😈

If you really want you can add app name option.

@crertel
Copy link
Contributor Author

crertel commented Feb 5, 2021

@Eiji7 went ahead and added that, and made some common helpers based on the Phoenix approach.

It'd still be cool to get some feedback from @boydm on this spike. :)

@crertel
Copy link
Contributor Author

crertel commented Feb 5, 2021

Thanks for looking over this with me @Eiji7 !

One bothersome detail is that the tests are failing for the gen.scenic.scene, because I don't think we're autodetecting the root module quite correctly. I've hit my spare cycles for this for today, but am open to ideas.

@Eiji7
Copy link

Eiji7 commented Feb 5, 2021

@crertel One bothersome detail is that the tests are failing for the gen.scenic.scene, because I don't think we're autodetecting the root module quite correctly. I've hit my spare cycles for this for today, but am open to ideas.

You have 2 options:

  1. Don't change generated app name (phoenix also checks for PhoenixWeb module name)
  2. Change configuration like:
Application.put_env(String.to_atom(@app_name), :namespace, String.to_atom(@module_name))

@Eiji7
Copy link

Eiji7 commented Feb 5, 2021

If you apply above suggestions to lib/mix/tasks/scenic.gen.component.ex and lib/mix/tasks/scenic.gen.scene.ex and then call mix format everything should be ok.

@Eiji7
Copy link

Eiji7 commented Feb 5, 2021

Before merge we would need a simple cleanup:

  1. Minimal callback documentation like @impl Scenic.Component (as said rest could be added later, but this is must have)
  2. Apply changes to pass mix format checks on Travis (already described above)
  3. Remove IO.inspect/1 call (added for debugging)
  4. Remove commented tests code or remove comments and update tests
  5. Following phoenix task file name should be task.name.ex for example: lib/mix/tasks/scenic.gen.component.ex
    see: https://github.com/phoenixframework/phoenix/tree/master/lib/mix/tasks
  6. Keep same file/directory structure in lib and test

lib/mix/tasks/scenic_gen_component.ex -> lib/mix/tasks/scenic.gen.component.exs
test/scenic_gen_component_test.exs -> test/mix/tasks/scenic.gen.component_test.exs

crertel and others added 5 commits February 5, 2021 10:54
Co-authored-by: Tomasz Marek Sulima <Eiji7@users.noreply.github.com>
Co-authored-by: Tomasz Marek Sulima <Eiji7@users.noreply.github.com>
crertel and others added 5 commits February 6, 2021 15:22
Co-authored-by: Tomasz Marek Sulima <Eiji7@users.noreply.github.com>
Co-authored-by: Tomasz Marek Sulima <Eiji7@users.noreply.github.com>
Co-authored-by: Tomasz Marek Sulima <Eiji7@users.noreply.github.com>
@Eiji7
Copy link

Eiji7 commented Feb 7, 2021

It would be nice if you could add 2 extra changes:

  def tmp_path do
    Path.expand("../tmp", __DIR__)
  end

  # …

  def in_tmp(which, function) do
    path = Path.join([tmp_path(), random_string(10), to_string(which)])

    try do
      File.rm_rf!(path)
      File.mkdir_p!(path)
      File.cd!(path, function)
    after
      path |> Path.dirname() |> File.rm_rf!()
    end
  end

The first in tmp_path/0 returns path to tmp dir in root project directory instead of parent directory which does not makes sense.

Secondly path |> Path.dirname() |> File.rm_rf!() removes not only Path.join([tmp_path(), random_string(10), to_string(which)]), but also Path.join([tmp_path(), random_string(10)]). That way we would not have many empty directories in tmp directory.

@Eiji7
Copy link

Eiji7 commented Feb 7, 2021

Regarding tests here goes for scene:

  @scene_module "TestScene"
  @scene_name "test_scene"

  test "gen.scene" do
    in_project("new with defaults", "my_app", fn _mix_project ->
      output = capture_io(fn -> Mix.Tasks.Scenic.Gen.Scene.run([@scene_module]) end)
      module_definition = "defmodule MyApp.Scene.#{@scene_module} do"
      assert_file("lib/scenes/#{@scene_name}.ex", module_definition)
      assert output =~ "Created scene #{@scene_module}."
    end)
  end

  test "gen.scene inside umbrella" do
    in_project("umbrella", "my_umbrella", ["--umbrella"], Mix.Tasks.New, fn _mix_project ->
      func = fn -> Mix.Tasks.Scenic.Gen.Scene.run([]) end
      assert_raise Mix.Error, func
    end)
  end

  test "gen.scene with app" do
    in_project("new with app", "my_app", ["--module", "My.App"], fn _mix_project ->
      args = [@scene_module, "--app", "My.App"]
      output = capture_io(fn -> Mix.Tasks.Scenic.Gen.Scene.run(args) end)
      assert output =~ "Created scene #{@scene_module}."
      module_definition = "defmodule My.App.Scene.#{@scene_module} do"
      assert_file("lib/scenes/#{@scene_name}.ex", module_definition)
    end)
  end

  test "gen.scene without args displays help" do
    in_project("help", "help", fn _mix_project ->
      output1 = capture_io(fn -> Mix.Tasks.Scenic.Gen.Scene.run([]) end)
      output2 = capture_io(fn -> Mix.Tasks.Help.run(["scenic.gen.scene"]) end)
      assert output1 == output2
    end)
  end

  defp in_project(tmp_dir, app_name, extra_args \\ [], task \\ Mix.Tasks.Scenic.New, project_func) do
    in_tmp(tmp_dir, fn ->
      capture_io(fn -> task.run([app_name | extra_args]) end)
      app_name |> String.to_atom() |> Mix.Project.in_project(app_name, project_func)
    end)
  end

@Eiji7
Copy link

Eiji7 commented Feb 7, 2021

And for component:

  @component_module "TestComponent"
  @component_name "test_component"

  test "gen.component" do
    in_project("new with defaults", "my_app", fn _mix_project ->
      output = capture_io(fn -> Mix.Tasks.Scenic.Gen.Component.run([@component_module]) end)
      module_definition = "defmodule MyApp.Component.#{@component_module} do"
      assert_file("lib/components/#{@component_name}.ex", module_definition)
      assert output =~ "Created component #{@component_module}."
    end)
  end

  test "gen.component inside umbrella" do
    in_project("umbrella", "my_umbrella", ["--umbrella"], Mix.Tasks.New, fn _mix_project ->
      func = fn -> Mix.Tasks.Scenic.Gen.Component.run([]) end
      assert_raise Mix.Error, func
    end)
  end

  test "gen.component with app" do
    in_project("new with app", "my_app", ["--module", "My.App"], fn _mix_project ->
      args = [@component_module, "--app", "My.App"]
      output = capture_io(fn -> Mix.Tasks.Scenic.Gen.Component.run(args) end)
      assert output =~ "Created component #{@component_module}."
      module_definition = "defmodule My.App.Component.#{@component_module} do"
      assert_file("lib/components/#{@component_name}.ex", module_definition)
    end)
  end

  test "gen.component without args displays help" do
    in_project("help", "help", fn _mix_project ->
      output1 = capture_io(fn -> Mix.Tasks.Scenic.Gen.Component.run([]) end)
      output2 = capture_io(fn -> Mix.Tasks.Help.run(["scenic.gen.component"]) end)
      assert output1 == output2
    end)
  end

  defp in_project(tmp_dir, app_name, extra_args \\ [], task \\ Mix.Tasks.Scenic.New, project_func) do
    in_tmp(tmp_dir, fn ->
      capture_io(fn -> task.run([app_name | extra_args]) end)
      app_name |> String.to_atom() |> Mix.Project.in_project(app_name, project_func)
    end)
  end

Look that in_project/5 is same for both, so should go to mix_helper.exs.

Those changes should satisfy codecov.

@crertel
Copy link
Contributor Author

crertel commented Feb 7, 2021

Okay, that should get us into a happier spot.

lib/common.ex Outdated Show resolved Hide resolved
@Eiji7
Copy link

Eiji7 commented Feb 7, 2021

After that remove WIP part from title as it would be ready to merge.

Co-authored-by: Tomasz Marek Sulima <Eiji7@users.noreply.github.com>
@crertel crertel changed the title [WIP] Add generators Add generators Feb 7, 2021
@crertel
Copy link
Contributor Author

crertel commented Feb 7, 2021

Okiedokie, let's get @boydm to take a look.

axelson and others added 2 commits October 17, 2021 13:40
Only update `new`, `gen_scene`, and `gen_component`

Switch to scenic_driver_local
Add a usage statement for gen_component
A few stylistic changes
Switch build_embedded to false to workaround a bug
Switch the scenic dep to the v0.11 branch (will need to be updated once
  a release is cut)
@crertel
Copy link
Contributor Author

crertel commented Oct 21, 2021

Broughtt in the awesome work @axelson did!

@Eiji7
Copy link

Eiji7 commented Oct 21, 2021

@crertel This code is unfortunately invalid now as v0.11 branch was deleted after merging PR. I guess we should mark it WIP until new scenic release, then change scenic version to latest from hex and finally mark it as ready to merge.

@crertel
Copy link
Contributor Author

crertel commented Oct 21, 2021

I'm doing cleanup right now, and switched us over to the hex v0.11 RC. More on the way.

@crertel
Copy link
Contributor Author

crertel commented Oct 21, 2021

@boydm Should be ready for proper consideration now!

@crertel
Copy link
Contributor Author

crertel commented Oct 21, 2021

Talked to Boyd today; we're going to move the generators over to single-file mix tasks in core scenic (and swap the templates for heredocs), and then move @axelson 's work here for 0.11 support into normal scenic_new.

@axelson
Copy link
Collaborator

axelson commented Oct 21, 2021

Sounds good 👍
Thanks for bringing in my changes!

@boydm
Copy link
Collaborator

boydm commented Oct 21, 2021

Talked to Cretel. The generators are better off in the main scenic tasks and should not go into scenic_new.

@axelson, sorry but I've done a bunch of work to scenic_new and now I'm thinking it would be too hard to merge your changes in. Take a look at the current version and lets talk if there are things you want to do to it.

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.

None yet

4 participants