Issue 9772046: Bring back tools.run(), but only conditionally on gflags.
|
Created:
10 years, 11 months ago by jcgregorio_google Modified:
10 years, 9 months ago CC:
google-api-python-client_googlegroups.com Visibility:
Public. |
DescriptionBring back tools.run(), but only conditionally on gflags.
Add a warning that tools.run() is deprecated and will be removed in a future version.
Patch Set 1 #Patch Set 2 : 80 cols #Patch Set 3 : ws #
Total comments: 8
Patch Set 4 : new file #Patch Set 5 : missing file #
Total comments: 2
Patch Set 6 : more back into tools.py #Patch Set 7 : remove old_run.py #Patch Set 8 : back to old_run.py #Patch Set 9 : missing file #
Total comments: 3
MessagesTotal messages: 17
Sign in to reply to this message.
https://codereview.appspot.com/9772046/diff/4001/oauth2client/tools.py File oauth2client/tools.py (right): https://codereview.appspot.com/9772046/diff/4001/oauth2client/tools.py#newcod... oauth2client/tools.py:110: def run2(flow, storage, flags, http=None): Use a different name that actually explains why there are two. I'm actually a fan of using that different name on the gflags version rather than here. https://codereview.appspot.com/9772046/diff/4001/oauth2client/tools.py#newcod... oauth2client/tools.py:236: import gflags We may as well put this in it's own file and conditionally import that file.
Sign in to reply to this message.
https://codereview.appspot.com/9772046/diff/4001/oauth2client/tools.py File oauth2client/tools.py (right): https://codereview.appspot.com/9772046/diff/4001/oauth2client/tools.py#newcod... oauth2client/tools.py:110: def run2(flow, storage, flags, http=None): On 2013/06/03 20:33:42, dhermes wrote: > Use a different name that actually explains why there are two. > Yeah, naming is tough, how about run_flow() or command_line_flow()? > I'm actually a fan of using that different name on the gflags version rather > than here. Using run() for the legacy version will keep old programs from breaking, at least until 1.3 when we drop run() completely. https://codereview.appspot.com/9772046/diff/4001/oauth2client/tools.py#newcod... oauth2client/tools.py:236: import gflags On 2013/06/03 20:33:42, dhermes wrote: > We may as well put this in it's own file and conditionally import that file. I'm not sure what that would gain us? This is code that traditionally lived in this file, and will be going away in the next release, so I'm trying to do the minimum here.
Sign in to reply to this message.
https://codereview.appspot.com/9772046/diff/4001/oauth2client/tools.py File oauth2client/tools.py (right): https://codereview.appspot.com/9772046/diff/4001/oauth2client/tools.py#newcod... oauth2client/tools.py:110: def run2(flow, storage, flags, http=None): Haha! "Naming is tough" (#understatement) run_flow SGTM On 2013/06/04 03:08:50, jcgregorio_google wrote: > On 2013/06/03 20:33:42, dhermes wrote: > > Use a different name that actually explains why there are two. > > > > Yeah, naming is tough, how about run_flow() or command_line_flow()? > > > I'm actually a fan of using that different name on the gflags version rather > > than here. > > Using run() for the legacy version will keep old programs from breaking, at > least until 1.3 when we drop run() completely. https://codereview.appspot.com/9772046/diff/4001/oauth2client/tools.py#newcod... oauth2client/tools.py:236: import gflags Conditionally defined code looks terrible and out of place. Also if we plan on nuking it soon, it will be EASIER to do if it is a matter of deleting one file and removing one import. On 2013/06/04 03:08:50, jcgregorio_google wrote: > On 2013/06/03 20:33:42, dhermes wrote: > > We may as well put this in it's own file and conditionally import that file. > > I'm not sure what that would gain us? This is code that traditionally lived in > this file, and will be going away in the next release, so I'm trying to do the > minimum here. >
Sign in to reply to this message.
https://codereview.appspot.com/9772046/diff/4001/oauth2client/tools.py File oauth2client/tools.py (right): https://codereview.appspot.com/9772046/diff/4001/oauth2client/tools.py#newcod... oauth2client/tools.py:110: def run2(flow, storage, flags, http=None): On 2013/06/04 04:00:50, dhermes wrote: > Haha! "Naming is tough" (#understatement) > > run_flow SGTM > > On 2013/06/04 03:08:50, jcgregorio_google wrote: > > On 2013/06/03 20:33:42, dhermes wrote: > > > Use a different name that actually explains why there are two. > > > > > > > Yeah, naming is tough, how about run_flow() or command_line_flow()? > > > > > I'm actually a fan of using that different name on the gflags version rather > > > than here. > > > > Using run() for the legacy version will keep old programs from breaking, at > > least until 1.3 when we drop run() completely. > Done. https://codereview.appspot.com/9772046/diff/4001/oauth2client/tools.py#newcod... oauth2client/tools.py:236: import gflags On 2013/06/04 04:00:50, dhermes wrote: > Conditionally defined code looks terrible and out of place. Also if we plan on > nuking it soon, it will be EASIER to do if it is a matter of deleting one file > and removing one import. > > On 2013/06/04 03:08:50, jcgregorio_google wrote: > > On 2013/06/03 20:33:42, dhermes wrote: > > > We may as well put this in it's own file and conditionally import that file. > > > > I'm not sure what that would gain us? This is code that traditionally lived in > > this file, and will be going away in the next release, so I'm trying to do the > > minimum here. > > > Done.
Sign in to reply to this message.
LGTM https://codereview.appspot.com/9772046/diff/13001/oauth2client/tools.py File oauth2client/tools.py (right): https://codereview.appspot.com/9772046/diff/13001/oauth2client/tools.py#newco... oauth2client/tools.py:236: from old_run import run Do we want a NameError for tools.run if the import fails? We could also set it to None or to a method which raises NotImplementedError.
Sign in to reply to this message.
Actually, moving the code to old_run is more difficult than I thought, the last copy of the code didn't work, I would need to move ClientRedirectHandler and ClientRedirectServer into their own file and import that into both tools.py and old_run.py, so I moved the code back into tools.py, which to me is the least ugly option. https://codereview.appspot.com/9772046/diff/13001/oauth2client/tools.py File oauth2client/tools.py (right): https://codereview.appspot.com/9772046/diff/13001/oauth2client/tools.py#newco... oauth2client/tools.py:236: from old_run import run On 2013/06/28 19:51:36, dhermes wrote: > Do we want a NameError for tools.run if the import fails? We could also set it > to None or to a method which raises NotImplementedError. Added a tools.run() that raises NotImplementedError().
Sign in to reply to this message.
On 2013/06/30 04:24:15, jcgregorio_google wrote: > Actually, moving the code to old_run is more difficult than I thought, the last > copy of the code didn't work, I would need to move ClientRedirectHandler and > ClientRedirectServer into their own file and import that into both tools.py and > old_run.py, so I moved the code back into tools.py, which to me is the least > ugly option. > > https://codereview.appspot.com/9772046/diff/13001/oauth2client/tools.py > File oauth2client/tools.py (right): > > https://codereview.appspot.com/9772046/diff/13001/oauth2client/tools.py#newco... > oauth2client/tools.py:236: from old_run import run > On 2013/06/28 19:51:36, dhermes wrote: > > Do we want a NameError for tools.run if the import fails? We could also set it > > to None or to a method which raises NotImplementedError. > > Added a tools.run() that raises NotImplementedError(). PTAL
Sign in to reply to this message.
If it's just ClientRedirectHandler and ClientRedirectServer you can do an import within the method to avoid circular imports. This makes the it so that the import is not bound to the module and hence just asks a tiny question to sys.modules. This way you can: from old_run import run in tools.py while still def run(...) # Import here to avoid circular dependency from tools import ClientRedirectHandler # pylint ignore this from tools import ClientRedirectServer # pylint ignore this in old_run.py. If there are other issues, we can discuss further, but the 140 lines of conditionally defined code is a bad habit we should avoid when we can (which we can here). On Sat, Jun 29, 2013 at 9:56 PM, <jcgregorio@google.com> wrote: > On 2013/06/30 04:24:15, jcgregorio_google wrote: > >> Actually, moving the code to old_run is more difficult than I thought, >> > the last > >> copy of the code didn't work, I would need to move >> > ClientRedirectHandler and > >> ClientRedirectServer into their own file and import that into both >> > tools.py and > >> old_run.py, so I moved the code back into tools.py, which to me is the >> > least > >> ugly option. >> > > > https://codereview.appspot.**com/9772046/diff/13001/** > oauth2client/tools.py<https://codereview.appspot.com/9772046/diff/13001/oauth2client/tools.py> > >> File oauth2client/tools.py (right): >> > > > https://codereview.appspot.**com/9772046/diff/13001/** > oauth2client/tools.py#**newcode236<https://codereview.appspot.com/9772046/diff/13001/oauth2client/tools.py#newcode236> > >> oauth2client/tools.py:236: from old_run import run >> On 2013/06/28 19:51:36, dhermes wrote: >> > Do we want a NameError for tools.run if the import fails? We could >> > also set it > >> > to None or to a method which raises NotImplementedError. >> > > Added a tools.run() that raises NotImplementedError(). >> > > PTAL > > https://codereview.appspot.**com/9772046/<https://codereview.appspot.com/9772... > -- Danny Hermes Developer Programs Engineer
Sign in to reply to this message.
On 2013/07/01 21:01:20, dhermes wrote: > If it's just ClientRedirectHandler and ClientRedirectServer you can do an > import within the method to avoid circular imports. This makes the it so > that the import is not bound to the module and hence just asks a tiny > question to sys.modules. > > This way you can: > from old_run import run > > in tools.py while still > > def run(...) > # Import here to avoid circular dependency > from tools import ClientRedirectHandler # pylint ignore this > from tools import ClientRedirectServer # pylint ignore this > > in old_run.py. > > > If there are other issues, we can discuss further, but the 140 lines of > conditionally defined code is a bad habit we should avoid when we can > (which we can here). > > > On Sat, Jun 29, 2013 at 9:56 PM, <mailto:jcgregorio@google.com> wrote: > > > On 2013/06/30 04:24:15, jcgregorio_google wrote: > > > >> Actually, moving the code to old_run is more difficult than I thought, > >> > > the last > > > >> copy of the code didn't work, I would need to move > >> > > ClientRedirectHandler and > > > >> ClientRedirectServer into their own file and import that into both > >> > > tools.py and > > > >> old_run.py, so I moved the code back into tools.py, which to me is the > >> > > least > > > >> ugly option. > >> > > > > > > https://codereview.appspot.**com/9772046/diff/13001/** > > > oauth2client/tools.py<https://codereview.appspot.com/9772046/diff/13001/oauth2client/tools.py> > > > >> File oauth2client/tools.py (right): > >> > > > > > > https://codereview.appspot.**com/9772046/diff/13001/** > > > oauth2client/tools.py#**newcode236<https://codereview.appspot.com/9772046/diff/13001/oauth2client/tools.py#newcode236> > > > >> oauth2client/tools.py:236: from old_run import run > >> On 2013/06/28 19:51:36, dhermes wrote: > >> > Do we want a NameError for tools.run if the import fails? We could > >> > > also set it > > > >> > to None or to a method which raises NotImplementedError. > >> > > > > Added a tools.run() that raises NotImplementedError(). > >> > > > > PTAL > > > > > https://codereview.appspot.**com/9772046/%3Chttps://codereview.appspot.com/97...> > > > > > > -- > Danny Hermes > Developer Programs Engineer Done.
Sign in to reply to this message.
https://codereview.appspot.com/9772046/diff/28001/oauth2client/old_run.py File oauth2client/old_run.py (right): https://codereview.appspot.com/9772046/diff/28001/oauth2client/old_run.py#new... oauth2client/old_run.py:28: from tools import ClientRedirectHandler I thought you said this wouldn't run if these were top level imports?
Sign in to reply to this message.
https://codereview.appspot.com/9772046/diff/28001/oauth2client/old_run.py File oauth2client/old_run.py (right): https://codereview.appspot.com/9772046/diff/28001/oauth2client/old_run.py#new... oauth2client/old_run.py:28: from tools import ClientRedirectHandler On 2013/07/02 18:16:59, dhermes wrote: > I thought you said this wouldn't run if these were top level imports? Yeah, seems to run fine, not sure what I was running into earlier.
Sign in to reply to this message.
https://codereview.appspot.com/9772046/diff/28001/oauth2client/old_run.py File oauth2client/old_run.py (right): https://codereview.appspot.com/9772046/diff/28001/oauth2client/old_run.py#new... oauth2client/old_run.py:28: from tools import ClientRedirectHandler Yeah I guess you're right. So long as "from old_run import run" happens after ClientRedirectHandler/ClientRedirectServer are defined, these values will be accessible on the module object in sys.modules. This makes it somewhat dangerous if the "from old_run import run" statement gets moved to before the ClientRedirect* definitions. Could you either add a comment above the "from old_run import run" block warning about this or do these "from tools import ClientRedirect*" conditionally within the method calls? On 2013/07/02 18:20:53, jcgregorio_google wrote: > On 2013/07/02 18:16:59, dhermes wrote: > > I thought you said this wouldn't run if these were top level imports? > > Yeah, seems to run fine, not sure what I was running into earlier. >
Sign in to reply to this message.
A good write-up on circular imports: http://stackoverflow.com/a/3956038/1068170
Sign in to reply to this message.
Yeah, we either have circular imports or large blocks of conditional code, both are unpleasant. This is all code that is going to be deleted shortly, and by shortly I mean the very first CL after v1.2 is released. On Tue, Jul 2, 2013 at 2:28 PM, <dhermes@google.com> wrote: > > https://codereview.appspot.**com/9772046/diff/28001/** > oauth2client/old_run.py<https://codereview.appspot.com/9772046/diff/28001/oauth2client/old_run.py> > File oauth2client/old_run.py (right): > > https://codereview.appspot.**com/9772046/diff/28001/** > oauth2client/old_run.py#**newcode28<https://codereview.appspot.com/9772046/diff/28001/oauth2client/old_run.py#newcode28> > oauth2client/old_run.py:28: from tools import ClientRedirectHandler > Yeah I guess you're right. So long as "from old_run import run" happens > after ClientRedirectHandler/**ClientRedirectServer are defined, these > values will be accessible on the module object in sys.modules. > > This makes it somewhat dangerous if the "from old_run import run" > statement gets moved to before the ClientRedirect* definitions. > > Could you either add a comment above the "from old_run import run" block > warning about this or do these "from tools import ClientRedirect*" > conditionally within the method calls? > > > On 2013/07/02 18:20:53, jcgregorio_google wrote: > >> On 2013/07/02 18:16:59, dhermes wrote: >> > I thought you said this wouldn't run if these were top level >> > imports? > > Yeah, seems to run fine, not sure what I was running into earlier. >> > > > https://codereview.appspot.**com/9772046/<https://codereview.appspot.com/9772... >
Sign in to reply to this message.
On 2013/07/02 18:33:22, jcgregorio_google wrote: > Yeah, we either have circular imports or large blocks of conditional code, > both are > unpleasant. This is all code that is going to be deleted shortly, and by > shortly I mean > the very first CL after v1.2 is released. There is a lot of documentation and code samples in this project that use tools.run. Will that be updated when tools.run is deleted?
Sign in to reply to this message.
All of the code samples have been updated, docs will be updated soon. On Tue, Jul 9, 2013 at 10:56 AM, <kevin@ie.suberic.net> wrote: > On 2013/07/02 18:33:22, jcgregorio_google wrote: > >> Yeah, we either have circular imports or large blocks of conditional >> > code, > >> both are >> unpleasant. This is all code that is going to be deleted shortly, and >> > by > >> shortly I mean >> the very first CL after v1.2 is released. >> > > There is a lot of documentation and code samples in this project that > use tools.run. Will that be updated when tools.run is deleted? > > https://codereview.appspot.**com/9772046/<https://codereview.appspot.com/9772... >
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
