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

fix: add decimal lib in dependencies #134

Closed

Conversation

davidjulien
Copy link
Contributor

@davidjulien davidjulien commented Sep 13, 2024

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

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.
@coveralls
Copy link

Coverage Status

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

@tompesman
Copy link

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

@davidjulien
Copy link
Contributor Author

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
Copy link
Member

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

@tompesman
Copy link

Awesome! 🚀

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

Successfully merging this pull request may close these issues.

4 participants