Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10933138:
Add a preliminary Python API for retrieving Google API keys. (Closed)
|
Created:
8 years, 3 months ago by Jói Modified:
8 years, 3 months ago Reviewers:
Jamie CC:
chromium-reviews, jamiewalch+watch_chromium.org, dcaiafa+watch_chromium.org, simonmorris+watch_chromium.org, hclam+watch_chromium.org, wez+watch_chromium.org, amit, sanjeevr, garykac+watch_chromium.org, lambroslambrou+watch_chromium.org, rmsousa+watch_chromium.org, alexeypa+watch_chromium.org, sergeyu+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd a preliminary Python API for retrieving Google API keys.
This has nearly identical semantics to the C++ API for builds where
the internal key file is available. For builds where that file is not
available, it does not yet have the same semantics, since at the
moment it does not have access to the gyp variables that would be
required (this is a TODO item). Instead, it will simply try to find
keys from environment variables, and if they are not found it will use
'dummytoken' as the default token for each key.
BUG=145584
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=157247
Patch Set 1 #
Total comments: 19
Patch Set 2 : Restrict to just API file, address review comments. #Patch Set 3 : . #
Total comments: 6
Patch Set 4 : Respond to review comments. #Patch Set 5 : Add DUMMY_TOKEN #Patch Set 6 : . #Patch Set 7 : . #Messages
Total messages: 7 (0 generated)
As requested on chat, I've only reviewed the library for now. http://codereview.chromium.org/10933138/diff/1/google_apis/google_api_keys.py File google_apis/google_api_keys.py (right): http://codereview.chromium.org/10933138/diff/1/google_apis/google_api_keys.py... google_apis/google_api_keys.py:39: line = line.strip() I'm not sure what the call to strip accomplishes here. At least, it will cause the next line to trigger incorrectly on a line ending in "\ ". http://codereview.chromium.org/10933138/diff/1/google_apis/google_api_keys.py... google_apis/google_api_keys.py:40: if line.strip().endswith('\\'): Duplicate call to split. http://codereview.chromium.org/10933138/diff/1/google_apis/google_api_keys.py... google_apis/google_api_keys.py:41: current_line += line[:-1] If you're going to handle multi-line defines, I tihnk you need to remove quotes to handle preprocessor string concatenation. http://codereview.chromium.org/10933138/diff/1/google_apis/google_api_keys.py... google_apis/google_api_keys.py:47: else: Not sure you need this final "else" since current_line is initialized to ''. http://codereview.chromium.org/10933138/diff/1/google_apis/google_api_keys.py... google_apis/google_api_keys.py:61: elif token_name in os.environ: I think this test would be better if it came before the official file test so that devs with src-internal checkouts who want to build with an unofficial key for some reason can do so. http://codereview.chromium.org/10933138/diff/1/google_apis/google_api_keys.py... google_apis/google_api_keys.py:82: if __name__ == "__main__": Is this test code, or should it stay?
PTAL https://codereview.chromium.org/10933138/diff/1/google_apis/google_api_keys.py File google_apis/google_api_keys.py (right): https://codereview.chromium.org/10933138/diff/1/google_apis/google_api_keys.p... google_apis/google_api_keys.py:39: line = line.strip() On 2012/09/17 22:28:55, Jamie wrote: > I'm not sure what the call to strip accomplishes here. At least, it will cause > the next line to trigger incorrectly on a line ending in "\ ". strip() only strips whitespace, so this is indeed to fix lines ending with e.g. "\ ". The duplicate doesn't matter but is an error as you pointed out. https://codereview.chromium.org/10933138/diff/1/google_apis/google_api_keys.p... google_apis/google_api_keys.py:40: if line.strip().endswith('\\'): On 2012/09/17 22:28:55, Jamie wrote: > Duplicate call to split. Done. https://codereview.chromium.org/10933138/diff/1/google_apis/google_api_keys.p... google_apis/google_api_keys.py:41: current_line += line[:-1] On 2012/09/17 22:28:56, Jamie wrote: > If you're going to handle multi-line defines, I tihnk you need to remove quotes > to handle preprocessor string concatenation. Oh, right, that's true, although in practice this is not a problem . Added a TODO for now, since the official keys header currently does not use string concatenation; is this OK with you? https://codereview.chromium.org/10933138/diff/1/google_apis/google_api_keys.p... google_apis/google_api_keys.py:47: else: On 2012/09/17 22:28:56, Jamie wrote: > Not sure you need this final "else" since current_line is initialized to ''. I do, since it's in a loop; I don't want current_line set to '' if the top condition (endswith \) is hit. https://codereview.chromium.org/10933138/diff/1/google_apis/google_api_keys.p... google_apis/google_api_keys.py:61: elif token_name in os.environ: On 2012/09/17 22:28:56, Jamie wrote: > I think this test would be better if it came before the official file test so > that devs with src-internal checkouts who want to build with an unofficial key > for some reason can do so. Changed. I was trying to stay as close to the semantics of the C++ API as possible, but for now this makes more sense. Once we can get the gyp variables in Python as well (my plan is to output the relevant ones to some file when chromium_gyp runs), then we can use the same approach as C++ devs would use, which is to set a gyp variable to not use the src-internal stuff even when it's there. https://codereview.chromium.org/10933138/diff/1/google_apis/google_api_keys.p... google_apis/google_api_keys.py:82: if __name__ == "__main__": On 2012/09/17 22:28:56, Jamie wrote: > Is this test code, or should it stay? It is test code, but it is a useful self test and there is nothing else obvious that the file should do when run as main, so I think it should just stay.
https://codereview.chromium.org/10933138/diff/1/google_apis/google_api_keys.py File google_apis/google_api_keys.py (right): https://codereview.chromium.org/10933138/diff/1/google_apis/google_api_keys.p... google_apis/google_api_keys.py:39: line = line.strip() On 2012/09/17 22:46:38, Jói wrote: > On 2012/09/17 22:28:55, Jamie wrote: > > I'm not sure what the call to strip accomplishes here. At least, it will cause > > the next line to trigger incorrectly on a line ending in "\ ". > > strip() only strips whitespace, so this is indeed to fix lines ending with e.g. > "\ ". The duplicate doesn't matter but is an error as you pointed out. My point was that lines ending with "\ " are not considered continuation lines by the preprocessor (I don't think...) but this script would treat them as such. Perhaps we don't care; I just wanted to make sure you were aware of it. https://codereview.chromium.org/10933138/diff/1/google_apis/google_api_keys.p... google_apis/google_api_keys.py:41: current_line += line[:-1] On 2012/09/17 22:46:38, Jói wrote: > On 2012/09/17 22:28:56, Jamie wrote: > > If you're going to handle multi-line defines, I tihnk you need to remove > quotes > > to handle preprocessor string concatenation. > > Oh, right, that's true, although in practice this is not a problem . Added a > TODO for now, since the official keys header currently does not use string > concatenation; is this OK with you? The problem is that the error will be very hard to detect, since this code will return the first part of the key and the error won't occur until run-time. Could you add something to cause a build failure in this case? For example, I think could you check for multiple quoted strings in ParseLine and throw an exception? https://codereview.chromium.org/10933138/diff/1/google_apis/google_api_keys.p... google_apis/google_api_keys.py:47: else: On 2012/09/17 22:46:38, Jói wrote: > On 2012/09/17 22:28:56, Jamie wrote: > > Not sure you need this final "else" since current_line is initialized to ''. > > I do, since it's in a loop; I don't want current_line set to '' if the top > condition (endswith \) is hit. I actually meant the entire else clause can go. If current_line is empty then it's identical to the previous one. I should more accurately have said that lines 43-47 inclusive can be deleted. https://codereview.chromium.org/10933138/diff/1/google_apis/google_api_keys.p... google_apis/google_api_keys.py:82: if __name__ == "__main__": On 2012/09/17 22:46:38, Jói wrote: > On 2012/09/17 22:28:56, Jamie wrote: > > Is this test code, or should it stay? > > It is test code, but it is a useful self test and there is nothing else obvious > that the file should do when run as main, so I think it should just stay. Fair enough. It will probably end up out-of-date if we add other client ids and secrets, but I guess the point is that we don't want to do that. https://codereview.chromium.org/10933138/diff/5002/google_apis/google_api_key... File google_apis/google_api_keys.py (right): https://codereview.chromium.org/10933138/diff/5002/google_apis/google_api_key... google_apis/google_api_keys.py:27: line_regexp = '^#define\s*%s\s*"([^"]+)"\s*$' % token_name FYI (I don't think it matters), but if the string has an embedded quote encoded as \" then this regex would fail. My previous suggestion about failing the build if the line is not formatted expected could probably cover this case as well. https://codereview.chromium.org/10933138/diff/5002/google_apis/google_api_key... google_apis/google_api_keys.py:61: Nit: I don't think this blank line adds to readability.
https://codereview.chromium.org/10933138/diff/5002/google_apis/google_api_key... File google_apis/google_api_keys.py (right): https://codereview.chromium.org/10933138/diff/5002/google_apis/google_api_key... google_apis/google_api_keys.py:66: return 'dummytoken' Could this string be exposed via a constant so that users of this module could detect failure without needing to duplicate it?
PTAL https://codereview.chromium.org/10933138/diff/1/google_apis/google_api_keys.py File google_apis/google_api_keys.py (right): https://codereview.chromium.org/10933138/diff/1/google_apis/google_api_keys.p... google_apis/google_api_keys.py:39: line = line.strip() On 2012/09/17 22:58:05, Jamie wrote: > On 2012/09/17 22:46:38, Jói wrote: > > On 2012/09/17 22:28:55, Jamie wrote: > > > I'm not sure what the call to strip accomplishes here. At least, it will > cause > > > the next line to trigger incorrectly on a line ending in "\ ". > > > > strip() only strips whitespace, so this is indeed to fix lines ending with > e.g. > > "\ ". The duplicate doesn't matter but is an error as you pointed out. > > My point was that lines ending with "\ " are not considered continuation lines > by the preprocessor (I don't think...) but this script would treat them as such. > Perhaps we don't care; I just wanted to make sure you were aware of it. Oh, I see, and I think you're right. The strip was also to remove the \n at the end of the line. I switched to removing two characters instead of one. https://codereview.chromium.org/10933138/diff/1/google_apis/google_api_keys.p... google_apis/google_api_keys.py:41: current_line += line[:-1] On 2012/09/17 22:58:05, Jamie wrote: > On 2012/09/17 22:46:38, Jói wrote: > > On 2012/09/17 22:28:56, Jamie wrote: > > > If you're going to handle multi-line defines, I tihnk you need to remove > > quotes > > > to handle preprocessor string concatenation. > > > > Oh, right, that's true, although in practice this is not a problem . Added a > > TODO for now, since the official keys header currently does not use string > > concatenation; is this OK with you? > > The problem is that the error will be very hard to detect, since this code will > return the first part of the key and the error won't occur until run-time. Could > you add something to cause a build failure in this case? For example, I think > could you check for multiple quoted strings in ParseLine and throw an exception? Done, although in a slightly different fashion than you suggested. https://codereview.chromium.org/10933138/diff/1/google_apis/google_api_keys.p... google_apis/google_api_keys.py:47: else: On 2012/09/17 22:58:05, Jamie wrote: > On 2012/09/17 22:46:38, Jói wrote: > > On 2012/09/17 22:28:56, Jamie wrote: > > > Not sure you need this final "else" since current_line is initialized to ''. > > > > I do, since it's in a loop; I don't want current_line set to '' if the top > > condition (endswith \) is hit. > > I actually meant the entire else clause can go. If current_line is empty then > it's identical to the previous one. I should more accurately have said that > lines 43-47 inclusive can be deleted. Ah, yes, thanks for that.
PTAL https://codereview.chromium.org/10933138/diff/5002/google_apis/google_api_key... File google_apis/google_api_keys.py (right): https://codereview.chromium.org/10933138/diff/5002/google_apis/google_api_key... google_apis/google_api_keys.py:27: line_regexp = '^#define\s*%s\s*"([^"]+)"\s*$' % token_name On 2012/09/17 22:58:05, Jamie wrote: > FYI (I don't think it matters), but if the string has an embedded quote encoded > as \" then this regex would fail. My previous suggestion about failing the build > if the line is not formatted expected could probably cover this case as well. Fixed. https://codereview.chromium.org/10933138/diff/5002/google_apis/google_api_key... google_apis/google_api_keys.py:61: On 2012/09/17 22:58:05, Jamie wrote: > Nit: I don't think this blank line adds to readability. Done. https://codereview.chromium.org/10933138/diff/5002/google_apis/google_api_key... google_apis/google_api_keys.py:66: return 'dummytoken' On 2012/09/17 23:06:41, Jamie wrote: > Could this string be exposed via a constant so that users of this module could > detect failure without needing to duplicate it? Done.
lgtm |
