Add a common check for `@doc` + `@spec` order

[joseemds]

This is a initial implementation, i still have to deal with some corner cases where Enum.chunk_every will not fit, but first i would like to receive some initial feedback if this implementation is a good beginning, and if it should report just the first case, or report all cases found.

Closes #181

[joseemds]

I added more tests and changed the implementation of DocOrderSpec.run, before it was using Enum.chunk_every(3) and now its using Enum.chunk_by(&(&1 in @def_ops)). Before the checker wouldnt work in a piece of code like this (Even if its wrong):

      defmodule Test do
        def a
        def b

        @doc ""
        @spec c
        def c
      end

Because this would be turned into [[:def, :def, :doc], [:spec, :def], and wouldnt match the old [:doc, :spec, op] clause.

Now it is chucking every time that other definition is found, with this that piece would turn into this: [[:def, :def], [:doc, :spec], [:def]], it is still not the most readable option, but it makes easier to separate definitions and module attributes, making easier to match the wrong order.

Please let me know if there is a better approach for this or if i should add more tests

[joseemds]

Hi @jiegillet thanks for your review! I added the check for different function and spec name, and also added params to both errors, i added they thinking that the template would be something like:

Wrong order: 

@spec @%{fn_name}
@doc ""
@%{@op} @%{fn_name}

Right order: 
@doc ""
@spec @%{fn_name}
@%{@op} @%{fn_name}

Spec and Function name doesnt match:
@spec @%{spec_name}
@%{@op} @%{fn_name}

It should be:
@spec @%{fn_name}
@%{@op} @%{fn_name}

But im not sure if this is the best way of doing it, please let me know if there is something to improve in the code or in the params errors!

[jiegillet]

The @ in the templates are not special characters, the string interpolation symbol is just %{} (I got it wrong my first time too :p ). We also prefer that the code in that code block be "actual" elixir code so things like "Right order:" should be commented.

If I were to do it, I think I would go for something simpler like this:

# recommended order
%{correct}

# your order
%{actual}

where correct and actual would be the only 2 parameters passed around and they would be created in the code. For example, correct could be the string @doc \"\"\n@spec function_name\ndefp function_name, etc.

[joseemds]

I changed the params to be correct and actual and will open a PR to the website-copy repo adding the markdown files soon. Let me know if there is something i can improve in the actual implementation or tests.

[joseemds]

Here is the PR adding the comments: https://github.com/exercism/website-copy/pull/2098

[jiegillet]

I'm OK with dropping the name-matching check if it makes the code murkier. We can always have another check for that.

[joseemds]

I agree, it will makes things simpler, now i will focus on just check the annotations order and add more tests, and later i can work on the spec and definition name checker

[joseemds]

So i renamed, changed the name and add some more tests, i think that it is pretty solid now, but im not sure if the error used should be the same for @doc and @spec order errors and for @doc or @spec between or after definition, what do you think?

[joseemds]

Hi @neenjaw, sorry for taking too long to answer. I pushed some commits with your tests and started considering the modules that the functions are in. About the error message, i think it is possible, not sure about the implementation, but i was thinking about actually printing the actual field based on the ops order, for eg:

# for %{name: :x, ops: [:def, :doc, :spec]} 

actual =  """
 def x
 @doc
 @spec x
"""

expected = """ 
  @doc  
  @spec x 
  def x
"""

This would make error messages more precise, but in this specific case it would not match very well with the description in the file:

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.

So i am not sure if i should add an error message exclusive for @spec or @doc after function definition or if i should change the current description. What do you think?

[neenjaw]

@joseemds, no worries, no problem about the timeframe.

I think we could reword the description to be more flexible:

There is no technical limitation placed on the order of the @doc and @spec attributes; however, the Elixir community has chosen a convention where the @doc is positioned first, then @spec, then finally the function definition.

If you think this is still awkward when the @doc and @spec appear after the function definition, I think it would be reasonable to limit this to only check situations where they are misordered but preceding the function definition.

[joseemds]

Hi @neenjaw, i have been thinking about it for a while and i think that would be better to limit the checker to check order only before or in the middle of the function definition, because this case is counterintuitive and awkward, so i think it will be rare to find someone doing it and also the implementation (at least in my head) will just get more complex, so it is probably not worth it.

[neenjaw]

@joseemds, I agree with you

[joseemds]

@neenjaw I fixed merge conflicts and some errors, and renamed the files in the website-copy PR, i think this is done, please let me know if i should change something

[angelikatyborska]

I feel weird as the only one with merge rights 🙂. I didn't involve myself in this PR because I think 2 reviewers is already enough, so I am happy to "blindly" approve and merge when @jiegillet too agrees it's ready (and @jiegillet can you also take care of merging the comment https://github.com/exercism/website-copy/pull/2098/files ?)

[joseemds]

I couldnt undestarnd why the tests failed, if you can point me the reason i can fix it!

[joseemds]

Done, now the tests are passing!

[jiegillet]

Yay! update the submodule and I'm merging :)

[joseemds]

done!

[jiegillet]

Thank you very much for your hard work over the last 2 months! @neenjaw, thank you as well for your all your valuable input.