Common check for @doc and @spec order doesn't handle private functions or private macros correctly

[petros]

I was working on the Name Badge exercise, where I decided to have a @spec for two private functions. In my editor, locally, I initially had a @doc and a @spec for those functions. ElixirLS informed me that the @doc strings will be stripped out because those are private functions. I decided to remove the @doc strings and only keep the @specs.

The code (SPOILER)

defmodule NameBadge do
  @separator " - "

  @doc """
  Take an id, a name, and a department and format a string
  that can be printed on a badge. It handles not having an ID which
  is the case for new employees. And it recognizes and onwer when
  no department is passed.
  """
  @spec print(integer() | nil, String.t(), String.t() | nil) :: String.t()
  def print(id, name, department) do
    format_id(id) <>
      name <>
      format_department(department)
  end

  @spec format_id(integer() | nil) :: String.t()
  defp format_id(id) do
    if is_nil(id), do: "", else: "[#{id}]" <> @separator
  end

  @spec format_department(String.t() | nil) :: String.t()
  defp format_department(department) do
    if is_nil(department),
      do: @separator <> "OWNER",
      else: @separator <> String.upcase(department)
  end
end

When I submitted my iteration, I got the following from the Elixir Analyzer:

Developers can choose the order of the @doc and @spec modules attributes, but the Elixir community convention is to use @doc first and @spec next to the function.

I do understand that, but why would I get it for a function that only has a @spec, and not a @doc string?

I would understand it if the recommendation was to also have a @doc string. But referring to the order of something that does not exist is confusing.

[angelikatyborska]

Thank you for the bug report! There's definitely something wrong. The check for the order of @doc and @spec seems not to handle private functions very well.

This code produces a comment about a wrong order when it shouldn't:

defmodule Test do
  @spec x()
  def x()

  @spec y()
  defp y()
end

This one as well:

defmodule Test do
  @spec x()
  def x()

  @spec y()
  defmacrop y()
end

and also this one:

defmodule Test do
  @spec x()
  def x()

  @spec y()
end

[Teilzeitdenker]

Well, I think one only has to change the last function in /elixir-analyzer/tree/main/lib/elixir_analyzer/exercise_test/common_checks/function_annotation_order.ex to

defp check_wrong_order(%{operations: operations}) do
    Enum.uniq(operations) not in [
      [],
      [:def],
      [:defp],
      [:defmacro],
      [:defmacrop],
      [:spec, :def],
      [:spec, :defp],
      [:spec, :defmacro],
      [:spec, :defmacrop],
      [:doc, :def],
      [:doc, :defp],
      [:doc, :defmacro],
      [:doc, :defmacrop],
      [:doc, :spec, :def],
      [:doc, :spec, :defp],
      [:doc, :spec, :defmacro]
      [:doc, :spec, :defmacrop]
    ]
  end
end

to also include all the cases where there are private functions or macros.

I'd be able to modify the corresponding files and write corresponding tests etc., but since I haven't been contributing here before, I'm not sure how I should go about it.

If I fork this repo and create a feature branch, how can I push this branch and create a pull request. Don't I have to be added as a collaborator first?

[petros]

If I fork this repo and create a feature branch, how can I push this branch and create a pull request. Don't I have to be added as a collaborator first?

You can create a pull request from your fork, that targets the main branch of the parent repository.

[angelikatyborska]

@Teilzeitdenker

Well, I think one only has to change the last function in (...) to also include all the cases where there are private functions or macros.

I think you would need to start here instead: https://github.com/exercism/elixir-analyzer/blob/5765dc1213c430c81cda8b1c008d659d05a170c4/lib/elixir_analyzer/exercise_test/common_checks/function_annotation_order.ex#L12 I think the main problem with the current code is that defp, defmacrop, defguard and defguardp don't trigger a cut off in the chain of found annotations. After that most likely check_wrong_order needs to be edited too.

If I fork this repo and create a feature branch, how can I push this branch and create a pull request. Don't I have to be added as a collaborator first?

You don't need to be added as a collaborator. First you need to create your own fork of this repository. You have all the necessary permissions to your own fork, and as @petros said, you can always open a pull request from your own fork into this repository. That doesn't require extra permissions.

[Teilzeitdenker]

Ok, I see. Thanks for the advice.

I'll give it a try and issue a pull request from my account if I'm able to fix it.

[Teilzeitdenker]

I issued a pull request, but I can't link it in here...