OpenAperture

Contributing to OpenAperture

This guide is intended to make the process of contributing to OpenAperture easy and efficient. As with OpenAperture, if you have suggestions/updates for this guide, please submit them here.

Checklist

Before submitting a pull request to OpenAperture, please complete the following:

• Tests exist for all new code
• Documentation exists for all new code
• No compiler warnings exist
• All tests complete
• Typespecs added/updated for all new/updated code
• Dialyzer warnings have been reviewed

Tests

All new code should have unit tests added that cover the new code. Tests that cover updated code should be checked to verify that the assumptions in the tests are still valid, and that the tests all still pass.

Documentation

All code should be documented using @moduledoc and @doc. Click here for more information on these annotations

Compiler Warnings

All warnings should be removed from the code. OpenAperture CI uses the mix compile --warnings-as-errors flag on automated builds, so code submitted with warnings will fail.

All tests complete

OpenAperture CI runs mix test, so make sure all tests are passing before submitting PR's.

Typespecs

Typespecs are used in OpenAperture in conjunction with Dialyzer to validate code. Click here for an introduction to typespecs in Elixir. Some notes:

• map, list, atom, and integer should be lowercase and should not end with .t
• Strings should be spec'd as String.t
• For OpenAperture, parens should not be used on types (e.g. String.t, not String.t()).
• Individual fields of Structs should be typespec'd like this:
      defmodule Person do
        defstruct name: nil, age: nil, gender: nil
        @type t :: %__MODULE__{name: String.t, age: integer, gender: :male | :female}
• If you are in a module that has a .t type defined, you should simply use the letter t as the type. e.g.:
        @spec change_name(t, String.t) :: t
• When multiple functions have the same compled parameter or return type, consider creating a custom type like this:
     @type my_type :: {:ok, integer} | {:error, String.t}

Dialyzer

Dialyzer is a static analyzis program, part of Erlang, which validates code using typespecs. Dialyzer is periodically run manually against OpenAperture code to verify that code is valid and root out potential issues. We use the dialyze project, which makes running Dialyzer as simple as running mix dialyze in the project root. It is not a requirement that code submitted to OpenAperture is free of Dialyzer warnings, however it is highly encouraged to review the warnings to ensure that there are no issues with the code that is being submitted.

Understanding Dialyzer

The first time you run dialyze with a given erlang-elixir version pair, it will take a while longer to run. This is because it is generating plt files for the erlang/elixir code, which it will then use on all subsequent executions. The output from dialyzer is quite cryptic, and it may take a while to learn what each one of the warnings mean. Remember that this is an erlang program, so the references will be to the erlang-generated versions of the module/function names (e.g. 'Elixir.OpenAperture.ErrorView':log_error()).

First off, dialyze will complain about any unknown types. Pay attention to these, you should be able to eliminate all of these warnings. For example, if it complains about "unknown type: 'Elixir.Map' ::t. this means you have a Map.t somewhere in your code that you should search for and convert to map. Complaining about an unknown type for a module in your code could mean that the @type t :: %{} line is missing from that module.

Second, any time in the code where you access a struct's fields, there will be a warning similar to this ("of type atom() | tuple() not #{}"):

lib/messaging/rpc_request.ex:118: The call _@8:'body'() requires that _@8 is of type atom() | tuple() not #{}

This is because of how elixir implements structs in erlang, and unfortunately there is no way to get around this warning. Nothing is being done incorrectly, so you can ignore these lines.

Next, whenever you see this warning ("has no local return"):

lib/messaging/amqp/connection_supervisor.ex:42: Function init/1 has no local return

it is a symptom of another warning that should be right before or after that warning. Once you fix the other warning, this will go away.

Any warnings that don't fall into the above categories are ones you actually will need to fix the typespec to eliminate.

The following warning indicates that you need to check the behavior you are implementing for a the correct return type, the one you specified doesn't match (check elixir/erlang docs for the right value):

lib/messaging.ex:49: The return type 'ignore' in the specification of start/2 is not a subtype of {'error',_} | {'ok',pid()} | {'ok',pid(),_}, which is the expected return type for the callback of application behaviour

The following warning ("the call XXX will never return") indicates that the calculated typespec of the paramters being passed into the function don't match the typespec of the function being called:

web/controllers/products/product_environmental_variables.ex:178: The call 'Elixir.OpenAperture.Manager.DB.Models.ProductEnvironmentalVariable':update(env_var@1::[any()],params@1::#{}) will never return since the success typing is (#{},#{}) -> #{} and the contract is ('Elixir.Ecto.Model':t(),params()) -> 'Elixir.Ecto.Changeset':t()

This warning ("breaks the contract") also means that the typespecs of the paramters don't match the typespecs of the function being called:

web/controllers/products/product_deployment_plans.ex:1: The call 'Elixir.Phoenix.Controller':put_new_layout(_@3::#{},{'Elixir.OpenAperture.LayoutView','application'}) breaks the contract ('Elixir.Plug.Conn':t(),{atom(),binary()} | 'false') -> 'Elixir.Plug.Conn':t()

This warning ("can never match the type") indicates that the case will never happen because the type of the case subject doesn't match the type of the case option (here, the case returns something that isnt a tuple, but the case option is a tuple):

web/controllers/products/product_deployment_plan_steps.ex:207: The pattern {_, _plan@1} can never match the type 'nil' | [any()]

This warning ("invalid type specification") means that the return type specified in the @spec differs from what it is actually returning:

web/controllers/products/product_deployment_plan_steps.ex:307: Invalid type specification for function 'Elixir.OpenAperture.Manager.Controllers.ProductDeploymentPlanSteps':get_product_and_plan_by_name/2. The success typing is (binary(),binary()) -> 'nil' | [any()]

There are other warnings you will run across, but hopefully this will be enough to get you up and running and thinking about typespecs and dialyzing.