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

Arguable advice from Credo.Check.Refactor.AppendSingleItem #1129

Open
elridion opened this issue May 7, 2024 · 1 comment
Open

Arguable advice from Credo.Check.Refactor.AppendSingleItem #1129

elridion opened this issue May 7, 2024 · 1 comment

Comments

@elridion
Copy link

elridion commented May 7, 2024

Environment

  • Credo version: 1.7.5-ref.main.81cc50fba+uncommittedchanges
  • Erlang/Elixir version: Erlang/OTP 26 [erts-14.1] [source] [64-bit] [smp:10:10] [ds:10:10:10] [async-threads:1] [jit] Elixir 1.15.6 (compiled with Erlang/OTP 26)
  • Operating system: OSX

The issue

The Credo.Check.Refactor.AppendSingleItem correctly flags a bunch of places where singles items are in fact appended to lists and that's all good.
Example from credo list = list_so_far ++ [new_item]

The issue is that the example states to use Enum.reverse/1 when order matters and gives the following adivce:

list = [new_item] ++ list_so_far
# ...
Enum.reverse(list)

The thing is that the given solution is somewhat wrong. If order matters and the list_so_far is something like [1, 2, 3] and we try to append 4 the resulting list looks like this[3, 2, 1, 4].

The correct solutions would be:

list = [new_item] ++ Enum.reverse(list_so_far)
# ...
Enum.reverse(list)

But I would argue that ist in fact messier than before.

Additionally

To add to that two possible functions come to mind to get around using ++ there would be

  • Enum.concat(list_so_far, [new_item]) or ...
  • List.insert_at(list_so_far, -1, new_item)

the thing is both are using the ++ themself. See Enum.concat and List.insert_at

@rrrene
Copy link
Owner

rrrene commented May 9, 2024

While I think that I am getting what you are trying to say, I am not sure what you suggest to do about it.

I understand why you are saying that the example given might be misunderstood. Yet, this hasn't come up in the 7 years this check exists.

If you have a suggestion for a better phrasing of the check's description, please share! ✌️

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

No branches or pull requests

2 participants