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

Regression in Mix.Tasks.Format.formatter_for_file introduced in 1.15.5 #12930

Closed
lukaszsamson opened this issue Sep 14, 2023 · 5 comments
Closed

Comments

@lukaszsamson
Copy link
Contributor

lukaszsamson commented Sep 14, 2023

Elixir and Erlang/OTP versions

Erlang/OTP 25 [erts-13.2.2.2] [source] [64-bit] [smp:12:12] [ds:12:12:10] [async-threads:1] [jit]

Elixir 1.15.5 (compiled with Erlang/OTP 25)

Operating system

macOS

Current behavior

Elixir 1.15.5 introduced a regression in formatter plugin loading making it no longer able to format ~H sigils.

Repro:

  1. mix phx.new formatter_regerssion
  2. cd formatter_regerssion
  3. mix deps.get
  4. break sigil formatting in lib/formatter_regerssion_web/components/core_components.ex
  5. execute this script in iex
Mix.start()
Code.compile_file("mix.exs")
Mix.Task.run("loadconfig")
{f, o} = Mix.Tasks.Format.formatter_for_file("lib/formatter_regerssion_web/components/core_components.ex")
text = File.read!("lib/formatter_regerssion_web/components/core_components.ex") 
text == f.(text)

on 1.15.5 it returns true
on 1.15.4 it compiles before and then returns false

Even if explicit Mix.Task.run("compile") is added on 1.15.5 it still produces a formatter not being able to format sigils.

Invoking Code.format_string! is also broken

IO.iodata_to_binary([Code.format_string!(text, o), ?\n]) == text
true

Most likely the regression was introduced in fixes for #12880 (9f977be or dc8cfcd)
Originally reported in elixir-lsp/elixir-ls#975

Expected behavior

Formatter should work when invoked via code. If plugin loading now requires some additional code this should be documented. Breaking changes should not be introduced in patch releases.

@josevalim
Copy link
Member

I will investigate. Please do test the fix once it is out, since the previous commit was aiming to address another bug in Elixir LSP and the breaking change is non intentional.

@lukaszsamson
Copy link
Contributor Author

I will investigate. Please do test the fix once it is out, since the previous commit was aiming to address another bug in Elixir LSP and the breaking change is non intentional.

The previous fix was not related to Elixir LS at all

@josevalim
Copy link
Member

@lukaszsamson oh, I thought that was the case, apologies. Anyway, please give it a try a now and let me know.

@lukaszsamson
Copy link
Contributor Author

Thanks @josevalim, It's working fine again on 6654f1b

@josevalim
Copy link
Member

Excellent, i will ship a new version either this week or the beginning of next one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

2 participants