elixir-tesla / tesla

The flexible HTTP client library for Elixir, with support for middleware and multiple adapters.
MIT License
2.01k stars 349 forks source link

Replace get_env with compile_env in module body & minor enhancement with version upgrade. #580

Closed uzairaslam196 closed 1 year ago

uzairaslam196 commented 1 year ago

close #579

Changes:

  1. Replace Application.get_env with Application.compile_env in Tesla.Middleware.Logger.Formatter & Tesla.Middleware.Telemetry modules.
  2. Put prefix with unused vars.
  3. Upgrade Elixir version to ~> 1.10.
  4. Upgrade Tesla version to 1.7.0.
yordis commented 1 year ago

Do you mind bumping the version in the mix.exs https://github.com/elixir-tesla/tesla/blob/30c3a298bc0de841d8929055429598342ef75573/mix.exs#L5

whatyouhide commented 1 year ago

You cannot do this while Tesla depends on Elixir ~> 1.7, since the compile_env functions were introduced in 1.10. I would recommend doing it conditionally:

# TODO: remove once we depend on Elixir 1.10+.
if function_exported?(Application, :compile_env, 3) do
  @config Application.compile_env(:tesla, __MODULE__, [])
else
  @config Application.get_env(:tesla, __MODULE__, [])
end
uzairaslam196 commented 1 year ago

You cannot do this while Tesla depends on Elixir ~> 1.7, since the compile_env functions were introduced in 1.10. I would recommend doing it conditionally:

# TODO: remove once we depend on Elixir 1.10+.
if function_exported?(Application, :compile_env, 3) do
  @config Application.compile_env(:tesla, __MODULE__, [])
else
  @config Application.get_env(:tesla, __MODULE__, [])
end

I have made changes accordingly. Please review

uzairaslam196 commented 1 year ago

Do you mind bumping the version in the mix.exs

https://github.com/elixir-tesla/tesla/blob/30c3a298bc0de841d8929055429598342ef75573/mix.exs#L5

@yordis what should be new version, i need your guidance on it. Should it be 1.6.2?

yordis commented 1 year ago

Related to https://github.com/elixir-tesla/tesla/pull/581#issuecomment-1546273797

I am OK change the supported Elixir version to be higher and follow a bit https://hexdocs.pm/elixir/compatibility-and-deprecations.html

@whatyouhide any objections?!

whatyouhide commented 1 year ago

@yordis absolutely not! I was just warning against a breaking change, but raising the required Elixir version to 1.10 is great, it's been around for quite a while now!

yordis commented 1 year ago

@whatyouhide I think I tried to say what you said 😄

@uzairaslam196 bump the version of elixir https://github.com/elixir-tesla/tesla/blob/30c3a298bc0de841d8929055429598342ef75573/mix.exs#L13 to be ~> 1.10 and the version https://github.com/elixir-tesla/tesla/blob/30c3a298bc0de841d8929055429598342ef75573/mix.exs#L5 to be 1.7.0

We wouldn't need to check for the func existence after that.

uzairaslam196 commented 1 year ago

@whatyouhide I think I tried to say what you said 😄

@uzairaslam196 bump the version of elixir

https://github.com/elixir-tesla/tesla/blob/30c3a298bc0de841d8929055429598342ef75573/mix.exs#L13

to be ~> 1.10 and the version https://github.com/elixir-tesla/tesla/blob/30c3a298bc0de841d8929055429598342ef75573/mix.exs#L5

to be 1.7.0 We wouldn't need to check for the func existence after that.

I got it. 😄

uzairaslam196 commented 1 year ago

@yordis i pushed new changes after upgrading versions, test cases are running fine. I used Elixir 1.12.3 with Erlang/OTP 24 locally.

yordis commented 1 year ago

@whatyouhide share some Code Review love 🙏🏻

@uzairaslam196 thank you so much for the help!