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

warning: method redefined; discarding old management_console_endpoint #671

Closed
pvdb opened this issue Nov 25, 2015 · 3 comments · Fixed by #678
Closed

warning: method redefined; discarding old management_console_endpoint #671

pvdb opened this issue Nov 25, 2015 · 3 comments · Fixed by #678

Comments

@pvdb
Copy link
Contributor

pvdb commented Nov 25, 2015

There's something really funky going on with the management_console_endpoint method, which is defined in two places:

  • Octokit::Default.management_console_endpoint
  • Octokit::Configurable#management_console_endpoint

To illustrate the issue:

$ ruby -W2 -I lib -r octokit -e exit
~/octokit.rb/lib/octokit/configurable.rb:111: warning: method redefined; discarding old management_console_endpoint
$ _

I can't say I understand the (TBH quite convoluted) code, but once I do I'll issue a PR, as one of the two versions is most likely superfluous and/or unused.

@pengwynn
Copy link
Collaborator

pengwynn commented Dec 5, 2015

@pvdb one of those modules provides the configurable Octokit settings (available both as module-level and instance-level settings), the other provides the default values for those settings. In this case, we may be able to call reset_agent in Octokit::Configurable#management_console_endpoint and drop Octokit::EnterpriseManagementConsoleClient#management_console_endpoint.

Thoughts @gjtorikian?

@gjtorikian
Copy link
Member

I believe we can go even simpler. Here's a diff that shuffles the attribute from accessor to simply writer:

diff --git a/lib/octokit/configurable.rb b/lib/octokit/configurable.rb
index a377523..b97a6c0 100644
--- a/lib/octokit/configurable.rb
+++ b/lib/octokit/configurable.rb
@@ -49,10 +49,10 @@ module Octokit

     attr_accessor :access_token, :auto_paginate, :client_id,
                   :client_secret, :default_media_type, :connection_options,
-                  :management_console_endpoint, :management_console_password,
+                  :management_console_password,
                   :middleware, :netrc, :netrc_file,
                   :per_page, :proxy, :user_agent
-    attr_writer :password, :web_endpoint, :api_endpoint, :login
+    attr_writer :password, :web_endpoint, :api_endpoint, :management_console_endpoint, :login

     class << self

This, including the separate Configurable and Client methods, is exactly the same way api_endpoint and web_endpoint are defined. I suspect we were trying to model those definitions but missed this attribute distinction.

With this diff the warning goes away. It looks like management_console_password can also drop down to writer, to match it counterpart, :password.

@pengwynn
Copy link
Collaborator

pengwynn commented Dec 7, 2015

Awesome, that should work. 👍

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 a pull request may close this issue.

3 participants