sugar-framework / sugar

Modular web framework for Elixir
https://sugar-framework.github.io/
MIT License
430 stars 29 forks source link

Warning on using Sugar on more recent versions of Elixir #86

Closed CharlesOkwuagwu closed 8 years ago

CharlesOkwuagwu commented 8 years ago

Hi, we get a few warnings when compiling Sugar on more recent versions of Elixir. I believe these can be avoided as the warnings suggest some simple solutions:

D:\Elixir>cd simple

D:\Elixir\simple>mix
Compiled lib/simple.ex
lib/simple/router.ex:2: warning: the variable "parsers_opts" is unsafe as it has been set in a conditional clause, as part of a case/cond/receive/if/&&/||. Please rewrite the clauses so the value is explicitly returned. For example:

    case int do
      1 -> atom = :one
      2 -> atom = :two
    end

Can be rewritten as:

    atom =
      case int do
        1 -> :one
        2 -> :two
      end

Compiled lib/pages/controllers/pages.ex
Compiled lib/simple/controllers/hello.ex
Compiled lib/simple/controllers/main.ex
lib/simple/router.ex:1: warning: the variable "opts" is unsafe as it has been set in a conditional clause, as part of a case/cond/receive/if/&&/||. Please rewrite the clauses so the value is explicitly returned. For example:

    case int do
      1 -> atom = :one
      2 -> atom = :two
    end

Can be rewritten as:

    atom =
      case int do
        1 -> :one
        2 -> :two
      end
...
...

Compiled lib/simple/router.ex
Generated simple app
Consolidated DBConnection.Query
Consolidated Poison.Decoder
Consolidated Plug.Exception
Consolidated List.Chars
Consolidated Ecto.DataType
Consolidated Ecto.Queryable
Consolidated Collectable
Consolidated Enumerable
Consolidated String.Chars
Consolidated IEx.Info
Consolidated Poison.Encoder
Consolidated Inspect
deps/templates/lib/sugar/templates/engines/eex.ex:13: warning: variable assigns is unused
deps/templates/lib/sugar/templates/engines/eex.ex:13: warning: redefining module :"Elixir.Sugar.Templates.CompiledTemplates.EEx.index.html.eex" (current version defined in memory)
deps/templates/lib/sugar/templates/engines/eex.ex:13: warning: variable assigns is unused

D:\Elixir\simple>
YellowApple commented 8 years ago

Are you using the "simple" app template (https://github.com/sugar-framework/simple-example)? If so, it's worth mentioning that said template hasn't been updated in almost a year (note to self: fix that). Notably, you'll want to update Sugar from 0.4.6 to 0.4.10 (in your mix.exs).

More to the point: do the warnings persist if you create a fresh Sugar project (following the getting started guide)?

CharlesOkwuagwu commented 8 years ago

No. Simple is just a name I choose for my project:

D:\Elixir>mix new simple

Also loading the latest version of Sugar

  defp deps do
    [
      #{ :sugar, "~> 0.4.6" }
      # Comment out the above line and uncomment the below one if you want the latest/greatest
       {:sugar, github: "sugar-framework/sugar"}
    ]
  end
CharlesOkwuagwu commented 8 years ago

It all works fine now, but this comes up each time you refresh the index page: localhost:4000

D:\Elixir\simple>iex --werl -S mix server

IEx(3)>
09:24:55.303 [error] #PID<0.431.0> running Simple.Router terminated
Server: localhost:4000 (http)
Request: GET /
** (exit) an exception was raised:
    ** (Plug.Conn.AlreadySentError) the response was already sent
        (plug) lib/plug/conn.ex:343: Plug.Conn.send_resp/1
        (simple) lib/simple/controllers/main.ex:1: Simple.Controllers.Main.call/2
        (simple) lib/simple/router.ex:1: Simple.Router.do_call/2
        (plug) lib/plug/adapters/cowboy/handler.ex:15: Plug.Adapters.Cowboy.Handler.upgrade/4
        (cowboy) src/cowboy_protocol.erl:442: :cowboy_protocol.execute/4

My router is very basic, hence I think this is coming from either use Sugar.Router or use Sugar.Controller

defmodule Simple.Router do
  use Sugar.Router
  plug Sugar.Plugs.HotCodeReload

  if Sugar.Config.get(:sugar, :show_debugger, false) do
    use Plug.Debugger, otp_app: :sugar_test
  end

  plug Plug.Static, at: "/static", from: :sugar_test

  # Uncomment the following line for session store
  # plug Plug.Session, store: :ets, key: "sid", secure: true, table: :session

  # Note the use of Plug.Parsers, and the use of the actual parsing
  # module (Plug.Parsers.MULTIPART) instead of an atom (:multipart)
  plug Plug.Parsers, parsers: [ Plug.Parsers.MULTIPART ]

  # Define your routes here
  get "/", Simple.Controllers.Main, :index
  post "/upload", Simple.Controllers.Main, :upload  # Note the POST
end

My controller is similarly very basic:

defmodule Simple.Controllers.Main do
  use Sugar.Controller

  def index(conn, []) do
    raw conn |> render #resp(200, "<h1>Hello guys</h1>")
  end

  def upload(conn, _) do
    require Logger
    Logger.debug(conn |> inspect)

    case conn.params["image"] do
      %Plug.Upload{filename: filename, path: path} ->
        {:ok, image} = File.read path
        {:ok, file} = File.open filename, [:write]
        IO.binwrite file, image
        File.close file

        conn |> render(filename: filename)  # Note use of render, not resp
      nil ->
        raw conn |> resp(400, "image was not attached!")
    end
  end
end
YellowApple commented 8 years ago

That usually happens on a refresh or page load immediately following a source code change. Not sure about a root cause yet beyond Sugar.Plugs.HotCodeReload. You can comment out line 2 of your router if it bugs you (at the expense of your app requiring a restart whenever you want a change to take effect), but the error shouldn't actually affect anything.

Closing this issue since the original problem is resolved. I've opened a new issue (sugar-framework/plugs#5) to track the Plug.Conn.AlreadySentError on code reload.

CharlesOkwuagwu commented 8 years ago

@YellowApple Erm, the linked above issue is totally unrelated to this problem.

Please recheck the "new" issue you opened related to this.

Thanks

YellowApple commented 8 years ago

Whoops. Meant sugar-framework/plugs#5. Sorry about that. Must've had some dyslexic moment there...

CharlesOkwuagwu commented 8 years ago

LOL, no worries. cheers.