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

Remove parens #90

Merged
merged 10 commits into from
Jul 7, 2024
Merged

Remove parens #90

merged 10 commits into from
Jul 7, 2024

Conversation

sodapopcan
Copy link
Contributor

@sodapopcan sodapopcan commented Jun 29, 2024

Here's my first draft of removing parens.

I should be functionally complete though I still have several things to do.

Most importantly, I'm looking for some guidance on testing and general file to use to get the config opts. I'm using Mix.Tasks.Format.formatter_opts_for_file("mix.exs") in the implementation and have added a non-exported locals_without_parens: [assert: 1] to this project's .formatter.exs just for testing purposes. I don't love this, but since I have no clean way to do DI I need to either do it this way, use a mock (which means adding a library), or do some env-checking. Perhaps there is another way I'm not thinking of. What do you think?

Other than that I need to:

  • Refactor.
  • Add docs.
  • Add more tests.

And once again I'd love some guidance on what to call this corrector.

Thanks!


@impl Recode.Task
def run(source, opts) do
formatter_opts = Mix.Tasks.Format.formatter_opts_for_file(".mix.exs")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should fetch the opts with the source path and a fallback.

Suggested change
formatter_opts = Mix.Tasks.Format.formatter_opts_for_file(".mix.exs")
formatter_opts = Mix.Tasks.Format.formatter_opts_for_file(source.path || "nofile.ex")

@NickNeck
Copy link
Member

Elixir has a bunch of defaults for locals without parens. We should also add these.

  @locals_without_parens [
    # Special forms
    alias: 1,
    alias: 2,
    case: 2,
    cond: 1,
    for: :*,
    import: 1,
    import: 2,
    quote: 1,
    quote: 2,
    receive: 1,
    require: 1,
    require: 2,
    try: 1,
    with: :*,

    # Kernel
    def: 1,
    def: 2,
    defp: 1,
    defp: 2,
    defguard: 1,
    defguardp: 1,
    defmacro: 1,
    defmacro: 2,
    defmacrop: 1,
    defmacrop: 2,
    defmodule: 2,
    defdelegate: 2,
    defexception: 1,
    defoverridable: 1,
    defstruct: 1,
    destructure: 2,
    raise: 1,
    raise: 2,
    reraise: 2,
    reraise: 3,
    if: 2,
    unless: 2,
    use: 1,
    use: 2,

    # Stdlib,
    defrecord: 2,
    defrecord: 3,
    defrecordp: 2,
    defrecordp: 3,

    # Testing
    assert: 1,
    assert: 2,
    assert_in_delta: 3,
    assert_in_delta: 4,
    assert_raise: 2,
    assert_raise: 3,
    assert_receive: 1,
    assert_receive: 2,
    assert_receive: 3,
    assert_received: 1,
    assert_received: 2,
    doctest: 1,
    doctest: 2,
    refute: 1,
    refute: 2,
    refute_in_delta: 3,
    refute_in_delta: 4,
    refute_receive: 1,
    refute_receive: 2,
    refute_receive: 3,
    refute_received: 1,
    refute_received: 2,
    setup: 1,
    setup: 2,
    setup_all: 1,
    setup_all: 2,
    test: 1,
    test: 2,

    # Mix config
    config: 2,
    config: 3,
    import_config: 1
  ]

Comment on lines 45 to 64
defp remove_parens(
locals_without_parens,
%Zipper{node: {fun, _, args} = node} = zipper,
issues,
true
) do
node =
Enum.reduce(locals_without_parens, node, fn
{^fun, arity}, node when length(args) == arity ->
{fun, meta, args} = node
meta = Keyword.delete(meta, :closing)

{fun, meta, args}

_, node ->
node
end)

{%Zipper{zipper | node: node}, issues}
end
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think about adding a function local_witout_parent?/3:

  defp local_without_parens?(locals_without_parens, fun, [_ | _] = args) do
    arity = length(args)

    Enum.any?(locals_without_parens, fn
      {^fun, :*} -> true
      {^fun, ^arity} -> true
      _other -> false
    end)
  end

  defp local_without_parens?(_locals_without_parens, _fun, _args), do: false

  defp remove_parens(
         locals_without_parens,
         %Zipper{node: {fun, meta, args}} = zipper,
         issues,
         true
       ) do
    if local_without_parens?(locals_without_parens, fun, args) do
      node = {fun, Keyword.delete(meta, :closing), args}
      {%Zipper{zipper | node: node}, issues}
    else
      {zipper, issues}
    end
  end

This allows us to simplify remove_parens/4 and observe the value :* in locals_without_parens.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Yes sorry, I should have marked this as a draft. The duplication was just to get it working and I'm happy to use this code! I did indeed observe :* and will have to look that up, though :)

end
end

defp remove_parens(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have two very similar implementations of remove_parens/4. We could give this function another function as the fourth arg instead of the boolean. So we can give remove_parens/4 different functions for opts[:autocorrect] == true and opts[:autocorrect] == false to do the appropriate job.

@NickNeck
Copy link
Member

Hi Andrew,
This looks great. Thanks again for your work. I've left you some comments. Have a nice Sunday.

@sodapopcan
Copy link
Contributor Author

Elixir has a bunch of defaults for locals without parens. We should also add these.

Oh, and looking at this I now understand :* which I did not know you can do!

When using Mix.Tasks.Format.formatter_opts_for_file("mix.exs") in my applications it returns the above list but doesn't here. I clearly need to better grok how this function works but will figure it out.

Thanks for all the feedback! I will finish this up later today.

@sodapopcan
Copy link
Contributor Author

I had some time before my dog walk this morning so I've implemented your feedback! Thanks, I learned some stuff there :)

There are only a few tests but I feel they are actually pretty sufficient as they cover the main cases. Anything more feels like I'm just testing Sourceror.

@NickNeck NickNeck merged commit a681e68 into hrzndhrn:main Jul 7, 2024
13 of 14 checks passed
@sodapopcan
Copy link
Contributor Author

Thanks for merging but I was planning on adding docs! I can still do that if you like though fine by me if you'd prefer to. Very sorry for the delay I got caught up in another personal project.

@NickNeck
Copy link
Member

NickNeck commented Jul 7, 2024

Oh, sorry. It would be great if you want to could add some docs. I would also like to ask you if it is ok if you to rename the task to LocalsWithoutParens. I have done this alright in main, but we could change the name again.

No worries about the delay, all is well and good. We have no deadlines here.

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

Successfully merging this pull request may close these issues.

2 participants