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

deprecate three methods for version 2.0 #1446

Merged
merged 3 commits into from
Aug 22, 2022

Conversation

hyuraku
Copy link
Contributor

@hyuraku hyuraku commented Aug 20, 2022

Description

apply Faraday#deprecate to three deprecated methods for version 2.0.

  • Request#method
  • Connection#token_auth
  • Connection#authorization

related with #1438

Todos

List any remaining work that needs to be done, i.e:

  • Tests
  • Documentation

Additional Notes

Optional section

@iMacTia
Copy link
Member

iMacTia commented Aug 20, 2022

Thank you @hyuraku, one thing I'm not entirely happy about is that the existing warnings not only communicate which method has been deprecated and from where it was called, but they also provide a hint as to how to address them, and a link to the relevant documentation.
Simply replacing them with Faraday::Deprecate will make that information disappear 😢

Do you think it would be possible to extend #deprecate so that it accepts an additional (optional) parameter that we can use to provide a "custom message" on some deprecation warnings, and use that new parameter in these cases?

@hyuraku
Copy link
Contributor Author

hyuraku commented Aug 20, 2022

@iMacTia

Do you think it would be possible to extend #deprecate so that it accepts an additional (optional) parameter that we can use to provide a "custom message" on some deprecation warnings, and use that new parameter in these cases?

I guess it's better to add custom_message as the args to Faraday::Deprecate#deprecate, How about?

  • Before change
    def deprecate(name, repl, ver)
         --< omitted >--
          msg = [
            "NOTE: #{target_message} is deprecated",
            repl == :none ? ' with no replacement' : "; use #{repl} instead. ",
            "It will be removed in or after version #{gem_ver}",
            "\n#{target}#{name} called from #{Gem.location_of_caller.join(':')}"
          ]
  • After change
def deprecate(name, repl, ver, custom_message)
       --< omitted >--
      msg = [
            "NOTE: #{target_message} is deprecated",
            repl == :none ? ' with no replacement' : "; use #{repl} instead. ",
            "It will be removed in or after version #{gem_ver}",
            custom_message
            "\n#{target}#{name} called from #{Gem.location_of_caller.join(':')}"
          ]
          
end

@iMacTia
Copy link
Member

iMacTia commented Aug 20, 2022

Yeah, that would be perfect to share the link to the documentation or any other free-text valuable context on the deprecation 🙏!

@hyuraku
Copy link
Contributor Author

hyuraku commented Aug 22, 2022

@iMacTia
Could you review this PR again?

Copy link
Member

@iMacTia iMacTia left a comment

Choose a reason for hiding this comment

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

Thank you @hyuraku for the changes and @olleolleolle for spotting the leftover line. LGTM!

@iMacTia iMacTia merged commit 2b1f331 into lostisland:1.x Aug 22, 2022
@hyuraku hyuraku deleted the 1.x_/deprecate_few_methods branch August 22, 2022 14:57
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.

None yet

3 participants