adobe / elixir-styler

An @elixir-lang code-style enforcer that will just FIFY instead of complaining
Apache License 2.0
635 stars 31 forks source link

Config styling separates code from comments #187

Open dfalling opened 1 month ago

dfalling commented 1 month ago

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 1.17.1 (compiled with Erlang/OTP 25)

* Styler: `locked at 1.0.0 (styler) 4ba5bc40`

## Example Input

```elixir
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:

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 commented 4 weeks ago

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 commented 4 weeks ago

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 commented 4 weeks ago

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 commented 3 weeks ago

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

novaugust commented 3 weeks ago

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 commented 7 hours ago

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