Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2767143005:
ntp_snippets: add script to fetch articles (Closed)
Descriptionntp_snippets: add script to fetch articles
I got tired of referring back to the cheat sheet, and encoding
experiments was annoying. This will hopefully be much better for
checking things.
There's a problem with --signed-in right now. Apparently the client id
and secret it auto-detects aren't usable. Providing our project's
internal one works fine, but I'll talk to Roger to see what's up with
the general client parameters.
Review-Url: https://codereview.chromium.org/2767143005
Cr-Commit-Position: refs/heads/master@{#459721}
Committed: https://chromium.googlesource.com/chromium/src/+/92b86718f51de0458b271b86ac19be4ba88afdae
Patch Set 1 #
Total comments: 8
Patch Set 2 : Review #Patch Set 3 : Handle expired token #
Total comments: 8
Patch Set 4 : Functions, exit status #Messages
Total messages: 15 (5 generated)
sfiera@chromium.org changed reviewers: + fhorschig@chromium.org
Here's what --short looks like: % components/ntp_snippets/remote/fetch.py --short Articles for you: “Trump-Russia inquiry in 'grave doubt'…” (The Guardian, 14h 38m ago) https://amp.theguardian.com/us-news/2017/mar/22/trump-russia-investigation-co... “Westbrook first to notch perfect triple-…” (ESPN, 10h 6m ago) http://www.espn.com/nba/story/_/id/18981152/russell-westbrook-oklahoma-city-t... “Lukas Podolski’s farewell stunner for…” (The Guardian, 16h 2m ago) https://amp.theguardian.com/football/2017/mar/22/germany-england-internationa... “The daredevils feeding a dangerous…” (BBC, 11h 55m ago) http://www.bbc.co.uk/news/amp/39353851 “Germany 1-0 England: five talking points…” (The Guardian, 15h 30m ago) https://amp.theguardian.com/football/blog/2017/mar/22/germany-1-0-england-fiv... “Here Are The Ages You Peak At Everything…” (IFLScience, 17h 39m ago) http://www.iflscience.com/editors-blog/here-are-the-ages-you-peak-at-everythi... “AT&T, other U.S. advertisers quit…” (USA TODAY, 19h 40m ago) http://amp.usatoday.com/story/99497194/ “Google's new feature is rather creepy…” (JOE, 15h 28m ago) https://www.joe.ie/tech/googles-new-feature-rather-creepy-lead-awkward-moment... “Here's Breath Of The Wild Running On A…” (kotaku.com, 15h 23m ago) http://kotaku.com/heres-breath-of-the-wild-running-on-a-pc-1793541571/amp “Chinese officials warned US bomber…” (cnn, 21h 29m ago) https://amp.cnn.com/cnn/2017/03/22/politics/china-us-aircraft-warned/index.html [More]
That's pretty cool! https://codereview.chromium.org/2767143005/diff/1/components/ntp_snippets/rem... File components/ntp_snippets/remote/fetch.py (right): https://codereview.chromium.org/2767143005/diff/1/components/ntp_snippets/rem... components/ntp_snippets/remote/fetch.py:74: storage = oauth2client.file.Storage(os.path.expanduser("~/.zineauth")) Could drop a line of documentation at the top of this file? (This looks like it could have permanent side effects for expired credentials ... or does 'get().invalid' check that? In this case, I would appreciate a comment for that) https://codereview.chromium.org/2767143005/diff/1/components/ntp_snippets/rem... components/ntp_snippets/remote/fetch.py:90: if args.short and not r.json().get("error"): when calling fetch.py --short -c staging: ValueError: No JSON object could be decoded Do we want to catch this and print the reason? https://codereview.chromium.org/2767143005/diff/1/components/ntp_snippets/rem... components/ntp_snippets/remote/fetch.py:104: creation_time, "%Y-%m-%dT%H:%M:%SZ") when calling fetch.py --short -c alpha: ValueError: time data '2017-03-23T13:22:27.744Z' does not match format '%Y-%m-%dT%H:%M:%SZ' https://codereview.chromium.org/2767143005/diff/1/components/ntp_snippets/rem... components/ntp_snippets/remote/fetch.py:149: def EncodeExperiments(experiments): Pretty magic. Maybe add a short comment what's the goal here?
https://codereview.chromium.org/2767143005/diff/1/components/ntp_snippets/rem... File components/ntp_snippets/remote/fetch.py (right): https://codereview.chromium.org/2767143005/diff/1/components/ntp_snippets/rem... components/ntp_snippets/remote/fetch.py:74: storage = oauth2client.file.Storage(os.path.expanduser("~/.zineauth")) On 2017/03/23 13:46:49, fhorschig wrote: > Could drop a line of documentation at the top of this file? > (This looks like it could have permanent side effects for > expired credentials ... or does 'get().invalid' check that? > In this case, I would appreciate a comment for that) I assume this is what the .invalid check is for. Added a comment at the top. https://codereview.chromium.org/2767143005/diff/1/components/ntp_snippets/rem... components/ntp_snippets/remote/fetch.py:90: if args.short and not r.json().get("error"): On 2017/03/23 13:46:49, fhorschig wrote: > when calling fetch.py --short -c staging: > > ValueError: No JSON object could be decoded > > Do we want to catch this and print the reason? The staging URL was wrong above (it's not a sandbox host). https://codereview.chromium.org/2767143005/diff/1/components/ntp_snippets/rem... components/ntp_snippets/remote/fetch.py:104: creation_time, "%Y-%m-%dT%H:%M:%SZ") On 2017/03/23 13:46:49, fhorschig wrote: > when calling fetch.py --short -c alpha: > > ValueError: time data '2017-03-23T13:22:27.744Z' does not match format > '%Y-%m-%dT%H:%M:%SZ' Fixed (sort of).
I am happy about the functionality, but have some minor readability proposals https://codereview.chromium.org/2767143005/diff/1/components/ntp_snippets/rem... File components/ntp_snippets/remote/fetch.py (right): https://codereview.chromium.org/2767143005/diff/1/components/ntp_snippets/rem... components/ntp_snippets/remote/fetch.py:149: def EncodeExperiments(experiments): On 2017/03/23 13:46:49, fhorschig wrote: > Pretty magic. Maybe add a short comment what's the goal here? Done ;D https://codereview.chromium.org/2767143005/diff/40001/components/ntp_snippets... File components/ntp_snippets/remote/fetch.py (right): https://codereview.chromium.org/2767143005/diff/40001/components/ntp_snippets... components/ntp_snippets/remote/fetch.py:65: url = API_HOSTS[args.component] + API_PATH How about moving everything in a function? def BuildRequest(args) # ... return url, headers So that we: url, headers = BuildRequest(args) r = request.post(url, headers=headers) Little work and a big increase in readability/reviewabilty? https://codereview.chromium.org/2767143005/diff/40001/components/ntp_snippets... components/ntp_snippets/remote/fetch.py:86: else: If in a separate function, this could be a guard clause instead of increasing the nesting quite significantly... https://codereview.chromium.org/2767143005/diff/40001/components/ntp_snippets... components/ntp_snippets/remote/fetch.py:94: if args.short and not r.json().get("error"): Same here: All following code depends on r and args.short. WDYT about a separate function? https://codereview.chromium.org/2767143005/diff/40001/components/ntp_snippets... components/ntp_snippets/remote/fetch.py:119: else: This is bugging me more than it should. WDYT about: if not args.short or r.json().get("error"): print(r.text) return # [the non-error, short case]
https://codereview.chromium.org/2767143005/diff/40001/components/ntp_snippets... File components/ntp_snippets/remote/fetch.py (right): https://codereview.chromium.org/2767143005/diff/40001/components/ntp_snippets... components/ntp_snippets/remote/fetch.py:65: url = API_HOSTS[args.component] + API_PATH On 2017/03/24 11:24:10, fhorschig wrote: > How about moving everything in a function? > > def BuildRequest(args) > # ... > return url, headers > > So that we: > url, headers = BuildRequest(args) > r = request.post(url, headers=headers) > > Little work and a big increase in readability/reviewabilty? Done. https://codereview.chromium.org/2767143005/diff/40001/components/ntp_snippets... components/ntp_snippets/remote/fetch.py:86: else: On 2017/03/24 11:24:10, fhorschig wrote: > If in a separate function, this could be a guard clause instead of increasing > the nesting quite significantly... Done. https://codereview.chromium.org/2767143005/diff/40001/components/ntp_snippets... components/ntp_snippets/remote/fetch.py:94: if args.short and not r.json().get("error"): On 2017/03/24 11:24:10, fhorschig wrote: > Same here: All following code depends on r and args.short. WDYT about a > separate function? Done. https://codereview.chromium.org/2767143005/diff/40001/components/ntp_snippets... components/ntp_snippets/remote/fetch.py:119: else: On 2017/03/24 11:24:10, fhorschig wrote: > This is bugging me more than it should. WDYT about: > > if not args.short or r.json().get("error"): > print(r.text) > return > > # [the non-error, short case] I actually made it more complicated because I remembered how annoying it is when command-line tools don't return a proper error code.
lgtm!
sfiera@chromium.org changed reviewers: + treib@chromium.org
+treib for proper OWNERS approval. Maybe you would prefer a new directory for scripts?
On 2017/03/24 16:24:53, sfiera wrote: > +treib for proper OWNERS approval. > > Maybe you would prefer a new directory for scripts? rs lgtm Nah, this folder is fine IMO. If we have many scripts at some point, we can consider moving them into their own folder.
The CQ bit was checked by sfiera@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1490603308481840,
"parent_rev": "9db40fe3d91bbaada5857d7360976d59bba943cc", "commit_rev":
"92b86718f51de0458b271b86ac19be4ba88afdae"}
Message was sent while issue was closed.
Description was changed from ========== ntp_snippets: add script to fetch articles I got tired of referring back to the cheat sheet, and encoding experiments was annoying. This will hopefully be much better for checking things. There's a problem with --signed-in right now. Apparently the client id and secret it auto-detects aren't usable. Providing our project's internal one works fine, but I'll talk to Roger to see what's up with the general client parameters. ========== to ========== ntp_snippets: add script to fetch articles I got tired of referring back to the cheat sheet, and encoding experiments was annoying. This will hopefully be much better for checking things. There's a problem with --signed-in right now. Apparently the client id and secret it auto-detects aren't usable. Providing our project's internal one works fine, but I'll talk to Roger to see what's up with the general client parameters. Review-Url: https://codereview.chromium.org/2767143005 Cr-Commit-Position: refs/heads/master@{#459721} Committed: https://chromium.googlesource.com/chromium/src/+/92b86718f51de0458b271b86ac19... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/92b86718f51de0458b271b86ac19... |
