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

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

Merged
merged 3 commits into from May 15, 2023

Conversation

uzairaslam196
Copy link
Contributor

@uzairaslam196 uzairaslam196 commented May 11, 2023

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

yordis commented May 12, 2023

Do you mind bumping the version in the mix.exs

@version "1.6.1"

@whatyouhide
Copy link
Contributor

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
Copy link
Contributor Author

uzairaslam196 commented May 12, 2023

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
Copy link
Contributor Author

Do you mind bumping the version in the mix.exs

@version "1.6.1"

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

@yordis
Copy link
Member

yordis commented May 12, 2023

Related to #581 (comment)

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

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

yordis commented May 13, 2023

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

@uzairaslam196 bump the version of elixir

tesla/mix.exs

Line 13 in 30c3a29

elixir: "~> 1.7",
to be ~> 1.10 and the version
@version "1.6.1"
to be 1.7.0

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

@uzairaslam196
Copy link
Contributor Author

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

@uzairaslam196 bump the version of elixir

tesla/mix.exs

Line 13 in 30c3a29

elixir: "~> 1.7",

to be ~> 1.10 and the version

@version "1.6.1"

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

I got it. 😄

- Elixir version to 1.10.
- Tesla version to 1.7.0
@uzairaslam196
Copy link
Contributor Author

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

Comment on lines +83 to +84
@disable_legacy_event Application.compile_env(:tesla, Tesla.Middleware.Telemetry,
disable_legacy_event: false)[:disable_legacy_event]
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
@disable_legacy_event Application.compile_env(:tesla, Tesla.Middleware.Telemetry,
disable_legacy_event: false)[:disable_legacy_event]
@disable_legacy_event Application.compile_env(:tesla [Tesla.Middleware.Telemetry, :disable_legacy_event], false)

I think this is a clearer implementation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I agree. Thanks for enhancement.

@yordis
Copy link
Member

yordis commented May 14, 2023

@whatyouhide share some Code Review love 🙏🏻

@uzairaslam196 thank you so much for the help!

Copy link
Contributor

@whatyouhide whatyouhide left a comment

Choose a reason for hiding this comment

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

Looks good to me too, but I'd merge #581 first so that this one can bump the version only once 🙃

@yordis yordis merged commit 27612d8 into elixir-tesla:master May 15, 2023
1 of 8 checks passed
@uzairaslam196 uzairaslam196 changed the title Replace get_env with compile_env in module body Replace get_env with compile_env in module body & minor enhancement with version upgrade. May 15, 2023
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.

Replace Application.get_env to Application.compile_env in module body
3 participants