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 warnings by conditionally compiling Decimal support #191

Merged
merged 3 commits into from
Jul 15, 2024

Conversation

fhunleth
Copy link
Contributor

This fixes the following Elixir 1.17 warnings when :decimal isn't
included in the dependency list:

==> jason
    warning: Decimal.new/1 is undefined (module Decimal is not available or is yet to be defined)
    │
 94 │         decimal.new(string)
    │                 ~
    │
    └─ (jason 1.4.3) lib/decoder.ex:94:17: Jason.Decoder.float_decode_function/1

warning: struct Decimal.Error is undefined (module Decimal.Error is not available or is yet to be defined)
└─ (jason 1.4.3) lib/decoder.ex: Jason.Decoder.float_decode_function/1

     warning: Decimal.to_string/2 is undefined (module Decimal is not available or is yet to be defined)
     │
 242 │     [?", decimal.to_string(value, :normal), ?"]
     │                  ~
     │
     └─ (jason 1.4.3) lib/encode.ex:242:18: Jason.Encode.struct/4

     warning: Decimal.to_string/1 is undefined (module Decimal is not available or is yet to be defined)
     │
 231 │     [?", decimal.to_string(value), ?"]
     │                  ~
     │
     └─ (jason 1.4.3) lib/encoder.ex:231:18: Jason.Encoder.Decimal.encode/2

This fixes the following Elixir 1.17 warnings when `:decimal` isn't
included in the dependency list:

```
==> jason
    warning: Decimal.new/1 is undefined (module Decimal is not available or is yet to be defined)
    │
 94 │         decimal.new(string)
    │                 ~
    │
    └─ (jason 1.4.3) lib/decoder.ex:94:17: Jason.Decoder.float_decode_function/1

warning: struct Decimal.Error is undefined (module Decimal.Error is not available or is yet to be defined)
└─ (jason 1.4.3) lib/decoder.ex: Jason.Decoder.float_decode_function/1

     warning: Decimal.to_string/2 is undefined (module Decimal is not available or is yet to be defined)
     │
 242 │     [?", decimal.to_string(value, :normal), ?"]
     │                  ~
     │
     └─ (jason 1.4.3) lib/encode.ex:242:18: Jason.Encode.struct/4

     warning: Decimal.to_string/1 is undefined (module Decimal is not available or is yet to be defined)
     │
 231 │     [?", decimal.to_string(value), ?"]
     │                  ~
     │
     └─ (jason 1.4.3) lib/encoder.ex:231:18: Jason.Encoder.Decimal.encode/2
```
Copy link
Owner

@michalmuskala michalmuskala left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm happy to merge this, thank you for working on this. That said, I think with some small changes we can make this even better

lib/decoder.ex Outdated
Comment on lines 89 to 100
if Code.ensure_loaded?(Decimal) do
defp float_decode_function(%{floats: :decimals}) do
fn string, token, skip ->
# silence xref warning
decimal = Decimal

try do
decimal.new(string)
rescue
Decimal.Error ->
token_error(token, skip)
end
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think at this point, we should just remove the old work-around for silencing the warnings

Suggested change
if Code.ensure_loaded?(Decimal) do
defp float_decode_function(%{floats: :decimals}) do
fn string, token, skip ->
# silence xref warning
decimal = Decimal
try do
decimal.new(string)
rescue
Decimal.Error ->
token_error(token, skip)
end
if Code.ensure_loaded?(Decimal) do
defp float_decode_function(%{floats: :decimals}) do
fn string, token, skip ->
try do
Decimal.new(string)
rescue
Decimal.Error ->
token_error(token, skip)
end

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Additionally, do you think it would make sense to have a function that would emit a nice error if this option is passed but decimal was not included? I think this would be a good usability upgrade.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think at this point, we should just remove the old work-around for silencing the warnings

Oh good. I almost did that and then wasn't 100% sure there wasn't some weird edge case. I added a commit to the PR that removes them.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Additionally, do you think it would make sense to have a function that would emit a nice error if this option is passed but decimal was not included? I think this would be a good usability upgrade.

Yes, that seems nice. I raised in the decoder with a message I think is consistent with the library. The encoder calls seemed fine since they required Decimal structs to be passed.

@michalmuskala michalmuskala merged commit f775592 into michalmuskala:master Jul 15, 2024
14 checks passed
@michalmuskala
Copy link
Owner

Thank you! ❤️

@fhunleth fhunleth deleted the fix-elixir-1_17-warnings branch July 15, 2024 14:11
@michalmuskala
Copy link
Owner

This was released in 1.4.4 today. Sorry for the delay

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.

2 participants