Nebo15 / logger_json

JSON logger formatter with support for Google Cloud, DataDog and other for Elixir.
https://nebo15.github.io/logger_json/
MIT License
237 stars 92 forks source link

fix: add decimal lib in dependencies #134

Closed davidjulien closed 3 weeks ago

davidjulien commented 4 weeks ago

Hello!

Currently, decimal is added indirectly because ecto is an (optional) dependency in mix.exs.

However, if the project where you want to add logger_json doesn't have ecto in its dependencies, decimal is not added and then logger_json doesn't compile:

==> logger_json
Compiling 17 files (.ex)
    error: Decimal.__struct__/0 is undefined, cannot expand struct Decimal. Make sure the struct name is correct. If the struct name exists and is correct but it still cannot be found, you likely have cyclic module usage in your code
    │
 39 │   def encode(%Decimal{} = decimal, _redactors), do: decimal
    │              ^
    │
    └─ lib/logger_json/formatter/redactor_encoder.ex:39:14: LoggerJSON.Formatter.RedactorEncoder.encode/2

== Compilation error in file lib/logger_json/formatter/redactor_encoder.ex ==
** (CompileError) lib/logger_json/formatter/redactor_encoder.ex: cannot compile module LoggerJSON.Formatter.RedactorEncoder (errors have been logged)
    lib/logger_json/formatter/redactor_encoder.ex:39: (module)
could not compile dependency :logger_json, "mix compile" failed. Errors may have been logged above. You can recompile this dependency with "mix deps.compile logger_json --force", update it with "mix deps.update logger_json" or clean it with "mix deps.clean logger_json"

You can easily reproduce the bug by removing the ecto dependency in the logger_json mix.exs file. You will see that the decimal dependency disappears in deps.tree and that the lib doesn't compile anymore (same error as above).

➜  mix deps.get
Resolving Hex dependencies...
Resolution completed in 0.055s
New:
  castore 1.0.8
  dialyxir 1.4.3
  earmark_parser 1.4.41
  erlex 0.2.7
  ex_doc 0.34.2
  excoveralls 0.18.3
  jason 1.4.4
  junit_formatter 3.4.0
  makeup 1.1.2
  makeup_elixir 0.16.2
  makeup_erlang 1.0.1
  mime 2.0.6
  nimble_parsec 1.4.0
  plug 1.16.1
  plug_crypto 2.1.0
  stream_data 1.1.1
  telemetry 1.3.0
All dependencies are up to date

It may be interesting to refactor the code in another PR so that decimal encoding is not compiled when decimal dependency doesn't exist in the project.

Regards

coveralls commented 4 weeks ago

Coverage Status

coverage: 100.0%. remained the same when pulling d7ebd67dafc83077e5fac0a638993c865c3e0787 on davidjulien:fix-missing-decimal-dependency into 13710322008c5f7102480d6f9eba650ebbb5ef9c on Nebo15:master.

tompesman commented 3 weeks ago

I think it should be an optional dependency. Not every project has the decimal dependency included.

davidjulien commented 3 weeks ago

I opened another PR where we checked if the Decimal module is compiled to decide if the compiler includes the clause related to it. It was easier than I thought initialy.

AndrewDryga commented 3 weeks ago

Merged a PR that checks if Decimal is present, thank you ❤️

tompesman commented 3 weeks ago

Awesome! 🚀