Config styling separates code from comments

[dfalling]

Hi, first off thanks for this great lib! I've been using it for quite a while now and really love the improvements over the stock formatter.

Issue

Styler 1.0.0's extensive reformatting of config files separates many code comments from the lines they're annotating.

Versions

• Elixir:

Erlang/OTP 27 [erts-15.0] [source] [64-bit] [smp:10:10] [ds:10:10:10] [async-threads:1] [jit]

Elixir 1.17.1 (compiled with Erlang/OTP 25)

• Styler: locked at 1.0.0 (styler) 4ba5bc40

Example Input

import Config

# Only in tests, remove the complexity from the password hashing algorithm
config :bcrypt_elixir, :log_rounds, 1

# We don't run a server during test. If one is required,
# you can enable the server option below.
config :ido, IdoWeb.Endpoint,
  http: [port: 4001],
  server: false

config :ido,
  env: :test

config :ex_aws,
  access_key_id: "bogus",
  secret_access_key: "also bogus"

# Print only error messages during test
config :logger, level: :error

# Make bcrypt fast (and unsafe) for testing only
config :bcrypt_elixir, log_rounds: 4

# Configure your database
config :ido, Ido.Repo,
  adapter: Ecto.Adapters.Postgres,
  types: Ido.PostgrexTypes,
  username: "postgres",
  password: "postgres",
  database: "ido_test",
  hostname: "localhost",
  pool: Ecto.Adapters.SQL.Sandbox

# Env variables
config :ido,
  photo_api: Ido.PhotoApiMock,
  email_parser: Ido.EmailParserMock,
  geocoding_api: Ido.GeocodingApiMock,
  photo_cdn: Ido.PhotoCDNMock,
  google_maps: Ido.GoogleMapsMock,
  sendgrid_inbound_key: "FAKE_API_KEY"

config :ido, Oban, testing: :inline

# increase login attempts to not interfere with tests
config :ido, max_login_attempts: 500

config :appsignal, :config, active: false

Stacktrace / Current Behaviour

import Config

config :appsignal, :config, active: false

# Only in tests, remove the complexity from the password hashing algorithm
config :bcrypt_elixir, :log_rounds, 1

# Make bcrypt fast (and unsafe) for testing only
config :bcrypt_elixir, log_rounds: 4

config :ex_aws,
  access_key_id: "bogus",
  secret_access_key: "also bogus"

# Configure your database
config :ido, Ido.Repo,
  adapter: Ecto.Adapters.Postgres,
  types: Ido.PostgrexTypes,
  username: "postgres",
  password: "postgres",
  database: "ido_test",
  hostname: "localhost",
  # We don't run a server during test. If one is required,
  # you can enable the server option below.
  pool: Ecto.Adapters.SQL.Sandbox

config :ido, IdoWeb.Endpoint,
  http: [port: 4001],
  server: false

config :ido, Oban, testing: :inline

config :ido,
  env: :test

# increase login attempts to not interfere with tests
# Env variables
config :ido, max_login_attempts: 500

config :ido,
  photo_api: Ido.PhotoApiMock,
  email_parser: Ido.EmailParserMock,
  geocoding_api: Ido.GeocodingApiMock,
  photo_cdn: Ido.PhotoCDNMock,
  google_maps: Ido.GoogleMapsMock,
  sendgrid_inbound_key: "FAKE_API_KEY"

# Print only error messages during test
config :logger, level: :error

Notice several blocks of code have been separated from their respective comments:

• # Env variables
• # We don't run a server during test. If one is required,

In my more complex config files (prod, runtime, dev), it's even messier.

Is there a different style of code commenting I should be using to prevent this? The majority of my config comments are scaffolded from the generators, so I won't be alone in hitting this.

[novaugust]

alas, sorry this got you. i have some loose heuristics that try to do the least damage possible, but don't currently have a good solution here. likely sourceror's comment algorithm would do a better job, but last i checked it's not fast enough to be run as part of mix format 🤔

on the upside, once you fix the comments up once styler won't keep throwing things around. it's just that first sort where things can 💥

thanks for the issue ❤️ this is at least more data to try to tune things to behave better

[dfalling]

Thanks for the response! I've manually fixed the comments for this styler upgrade. Sadly it's something that can sneakily resurface when new config with comments is added in the future. Devs have to be attentive in PRs to make sure the comments go with the code.

[novaugust]

Sadly it's something that can sneakily resurface when new config with comments is added in the future

I'd be really interested in a repro for this, haven't experienced it myself. Likely because I'm only working in enormous config files...

[dfalling]

A fresh mix phx.gen.new will reproduce this.

[novaugust]

Right, i mean specifically for "can sneakily resurface when new config with comments is added". I'd love one where adding one or two new config messes up an already-sorted config

[novaugust]

came up with my own repro yesterday, sad bug indeed. no easy fix currently, so this may unfortunately sit a while longer

[mxgrn]

Good I didn't have to submit this :) Happened to me, too:

# Initialize plugs at runtime for faster test compilation
config :phoenix, :plug_init_mode, :runtime

config :pairs_one, :telegram_bot,
  token: "9999999999:AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA"

===>

config :pairs_one, :telegram_bot,
  # Initialize plugs at runtime for faster test compilation
  token: "9999999999:AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA"

config :phoenix, :plug_init_mode, :runtime

FWIW: if I put this part onto a single line, it works correctly:

config :pairs_one, :telegram_bot,
  token: "9999999999:AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA"

[jakeprem]

Would you consider adding an option to disable Styler formatting in config files?
Not sure if that's technically feasible or fits the vision for the library but seems like the simplest solution here.

Actually this is possible without any changes to this library, it can be done with standard .formatter.exs config.

Update your root .formatter.exs

# .formatter.exs
[
    subdirectories: ["config", ...],
    # make sure to remove config from :inputs as well (it is added by default).
    # See the docs for the :subdirectories option
    inputs: [...] 
    ...
]

And add config/.formatter.exs

# config/.formatter.exs
[
  inputs: ["*.exs"]
]

https://hexdocs.pm/mix/Mix.Tasks.Format.html#module-formatting-options

[novaugust]

yeah that's definitely something i'd consider to work-around comments going willy-nilly
removing configs from formatter.exs means formatter won't run on them anymore either, which is probably undesirable for most folks =)

[novaugust]

before making configs enable/disable-able, i'm releasing 1.1.2 which removes a poor heuristic that was causing lots of comments moving. it still won't be perfect but i expect will be much better vis-a-vis small additions to config files.

(this change fixed the issue for us where a single line being added to a large config caused all comments to be moved to the end of the file. instead, the new line was sorted and no other changes were made)

[novaugust]

oops, the above comment was a bit opaque. what i meant was
if this fix still leaves folks with lots of issues, i'll look at an enable/disable config. but i hope that this fixes the problem 🤞

[jakeprem]

yeah that's definitely something i'd consider to work-around comments going willy-nilly removing configs from formatter.exs means formatter won't run on them anymore either, which is probably undesirable for most folks =)

Just for clarity, the config above still has the Elixir formatter running on config/*, it just skips Styler (and any other config plugins that aren't specifically added to config/.formatter.exs.

Appreciate you looking into this and the heuristic update!

[novaugust]

Just for clarity, the config above still has the Elixir formatter running on config/*, it just skips Styler (and any other config plugins that aren't specifically added to config/.formatter.exs.

Whoaaa totally was not aware you could set different configurations within a project like that, so it did indeed go over my head what you were saying. Very cool, thanks for the tip!

Appreciate you looking into this and the heuristic update!

For sure. It was making us sad internally too, just had to have the time to get to it :)