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

Mix.Tasks.Format.formatter_for_file is broken in umbrella #12969

Closed
lukaszsamson opened this issue Sep 28, 2023 · 7 comments
Closed

Mix.Tasks.Format.formatter_for_file is broken in umbrella #12969

lukaszsamson opened this issue Sep 28, 2023 · 7 comments

Comments

@lukaszsamson
Copy link
Contributor

Elixir and Erlang/OTP versions

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

Elixir 1.15.6 (compiled with Erlang/OTP 26)

Operating system

macOS

Current behavior

Repro:

  1. mix phx.new formatter_umbrella --umbrella (install deps)
  2. cd formatter_umbrella
  3. run this iex
Mix.start()
Code.compile_file("mix.exs")
path = "apps/formatter_umbrella_web/lib/formatter_umbrella_web/components/core_components.ex"
{f, o} = Mix.Tasks.Format.formatter_for_file(path)

Observe that returned formatter options are for the wrong umbrella app. Instead of options for formatter_umbrella_web formatter_umbrella is returned. If you make an edit to apps/formatter_umbrella/.formatter.exs and e.g. remove plugins and get options again it will return invalid options

[
   sigils: [],
   import_deps: [:ecto, :ecto_sql],
   subdirectories: ["priv/*/migrations"],
   inputs: ["*.{heex,ex,exs}", "{config,lib,test}/**/*.{heex,ex,exs}",
    "priv/*/seeds.exs"],
    ...

the returned function will not be able to format contents of ~H.

Interestingly, this is able to format correctly

Mix.Task.run("format")

Expected behavior

The function should work correctly in umbrella apps

@josevalim
Copy link
Member

I cannot reproduce this behaviour. I got this in both cases:

{#Function<12.58511805/1 in Mix.Tasks.Format.find_formatter_for_file/2>,
 [
   sigils: [H: #Function<29.58511805/2 in Mix.Tasks.Format.load_plugins/1>],
   import_deps: [:ecto, :ecto_sql],
   subdirectories: ["priv/*/migrations"],
   plugins: [Phoenix.LiveView.HTMLFormatter],
   inputs: ["*.{heex,ex,exs}", "{config,lib,test}/**/*.{heex,ex,exs}",

Which seems correct to me and includes the relevant sigils.

Info:

$ mix phx.new --version
Phoenix installer v1.7.7

$ iex
Erlang/OTP 26 [erts-14.0.2] [source] [64-bit] [smp:10:10] [ds:10:10:10] [async-threads:1] [jit]

Interactive Elixir (1.15.6) - press Ctrl+C to exit (type h() ENTER for help)

@josevalim
Copy link
Member

Keep in mind that latest Phoenix versions added this to the root mix.exs:

  defp deps do
    [
      # Required to run "mix format" on ~H/.heex files from the umbrella root
      {:phoenix_live_view, ">= 0.0.0"}
    ]
  end

if you haven't updated phx.new, you may be missing it.

@lukaszsamson
Copy link
Contributor Author

Keep in mind that latest Phoenix versions added this to the root mix.exs:

I have that in root mix.exs and I am using phx.new v1.7.7

I cannot reproduce this behaviour

@josevalim you did reproduce it. Please notice

import_deps: [:ecto, :ecto_sql],
subdirectories: ["priv/*/migrations"]

this comes from apps/formatter_umbrella/.formatter.exs and

import_deps: [:phoenix]

from apps/formatter_umbrella_web/.formatter.exs is not there

@josevalim
Copy link
Member

Hrm, thanks. I was mostly looking at the sigils. It seems we are getting mixed results then?

@josevalim
Copy link
Member

@lukaszsamson ah, that was interesting, it only happens if one app name is the prefix of the other. A temporary fix for now is to swap the subdirectories order and have the longer name come first. I pushed a fix as well, please give it a try and thanks for the repro steps!

@lukaszsamson
Copy link
Contributor Author

I tried out the patch on 1.15 brnch and now it's working as expected. Thanks

lukaszsamson added a commit to elixir-lsp/elixir-ls that referenced this issue Sep 28, 2023
@josevalim
Copy link
Member

Sweeeeeeeeeet, thanks!

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