firetactoe pull request by mogar1980 · Pull Request #558 · GoogleCloudPlatform/python-docs-samples

mogar1980 · 2016-10-05T23:15:05Z

Here's the pull request. Includes all the python and html source files for firetactoe

googlebot · 2016-10-05T23:15:07Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please let us know the company's name.

waprin · 2016-10-05T23:34:13Z

appengine/standard/firebase/firetactoe/fire_index.html

+  	var config = {
+    	apiKey: "AIzaSyB_pwNiBi8Q3iakJitkQHtEQ5SZWmp9EiY",
+    	authDomain: "imposing-mind-131903.firebaseapp.com",
+    	databaseURL: "https://imposing-mind-131903.firebaseio.com"


probably best to put placeholders here, preferably put all this stuff in a single file that gets pulled in somehow.

mogar1980 · 2016-10-05T23:34:14Z

I signed it!

On Wed, Oct 5, 2016 at 4:15 PM, googlebot notifications@github.com wrote:

Thanks for your pull request. It looks like this may be your first
contribution to a Google open source project. Before we can look at your
pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/
https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll

verify. Thanks.

If you've already signed a CLA, it's possible we don't have your
GitHub username or you're using a different email address. Check your
existing CLA data https://cla.developers.google.com/clas and verify
that your email is set on your git commits
https://help.github.com/articles/setting-your-email-in-git/.

If you signed the CLA as a corporation, please let us know the
company's name.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#558 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AUBaDkVde1YCvfDwQx1iozze1_d8QVpVks5qxC9-gaJpZM4KPYyz
.

googlebot · 2016-10-05T23:34:15Z

CLAs look good, thanks!

waprin · 2016-10-05T23:34:37Z

Assuming there's more context for this PR someone else knows about?

mogar1980 · 2016-10-05T23:36:23Z

Absolutely! I work at Google, actually. Keys/secrets are in there so that
DPE folks can get everything to work while they test, debug, etc. The final
version of the sample definitely won't include these.

On Wed, Oct 5, 2016 at 4:34 PM, Bill Prin notifications@github.com wrote:

Assuming there's more context for this PR someone else knows about?

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#558 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AUBaDk1HWLPWUrCcO5uEamOVPYIvJmGCks5qxDQPgaJpZM4KPYyz
.

waprin · 2016-10-05T23:39:17Z

appengine/standard/firebase/firetactoe/firetactoe.py

@@ -0,0 +1,288 @@
+#!/usr/bin/python2.4
+#


needs license header

applies to every file

waprin · 2016-10-05T23:40:03Z

Ok cool. I would still refactor out all the config and secrets and add a README on how to generate them. We will need that anyway, so no good reason not to do it now before merge.

waprin · 2016-10-05T23:40:30Z

appengine/standard/firebase/firetactoe/fire_index.html

+
+      <style type='text/css'>
+        body {
+          font-family: 'Helvetica';


think we should also factor out css and js into separate files.

waprin · 2016-10-05T23:43:17Z

appengine/standard/firebase/firetactoe/fire_index.html

+          var square = document.getElementById(i);
+          square.innerHTML = state.board[i];
+          if (state.winner != '' && state.winningBoard != '') {
+            if (state.winningBoard[i] == state.board[i]) {


Believe we should be using triple =. Have you run this through jslint?

mogar1980 · 2016-10-05T23:46:31Z

All good points. Is there any way you can hand my review over to jonwayne@
who's going to be taking over this code sample?

Thanks!
Morgan

On Wed, Oct 5, 2016 at 4:43 PM, Bill Prin notifications@github.com wrote:

@waprin commented on this pull request.

In appengine/standard/firebase/firetactoe/fire_index.html
#558 (review)
:
<script type='text/javascript'>
 var state = {
   game_key: '{{ game_key }}',
   me: '{{ me }}'
 };
 // This is our Firebase realtime DB path that we'll listen to for updates
 // We'll initialize this later in openChannel()
 var channel = null;
 updateGame = function() {
   for (i = 0; i < 9; i++) {
     var square = document.getElementById(i);
     square.innerHTML = state.board[i];
     if (state.winner != '' && state.winningBoard != '') {
       if (state.winningBoard[i] == state.board[i]) {
Believe we should be using triple =. Have you run this through jslint?

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#558 (review),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AUBaDvWIj1eHIvVwFlDXxh7uj_PPuF2mks5qxDYXgaJpZM4KPYyz
.

theacodes · 2016-10-06T16:32:57Z

All good points. Is there any way you can hand my review over to jonwayne@
who's going to be taking over this code sample?

Yep, I'm here. Bill just beat me to the punch. 👍 I'm gonna add some comments but definite want you to address what bill has mentioned as well.

theacodes

Finished first pass, please ping me when you're ready for the next round of review.

theacodes · 2016-10-06T16:34:25Z

appengine/standard/firebase/firetactoe/app.yaml

@@ -0,0 +1,11 @@
+runtime: python27


(Github won't let me comment on a binary file): Please remove .DS_Store. I'd recommend adding it to your global gitignore.

theacodes · 2016-10-06T16:34:56Z

appengine/standard/firebase/firetactoe/app.yaml

+
+libraries:
+- name: pycrypto
+  version: latest


Always use explicit versions. Latest sometimes isn't the latest, and even if it were we don't want the sample to suddenly break.

theacodes · 2016-10-06T16:35:55Z

appengine/standard/firebase/firetactoe/fire_index.html

+  <head>
+
+  <script src="https://www.gstatic.com/firebasejs/3.3.2/firebase.js"></script>
+  <!-- <script src="https://www.gstatic.com/firebasejs/3.3.0/firebase-database.js"></script> -->


If this isn't necessary, remove it.

theacodes · 2016-10-06T16:36:09Z

appengine/standard/firebase/firetactoe/fire_index.html

+
+  <script src="https://www.gstatic.com/firebasejs/3.3.2/firebase.js"></script>
+  <!-- <script src="https://www.gstatic.com/firebasejs/3.3.0/firebase-database.js"></script> -->
+  <script>


Please move all this to a separate .js file.

theacodes · 2016-10-06T16:36:34Z

appengine/standard/firebase/firetactoe/fire_index.html

+      </style>
+  </head>
+  <body>
+    <script type='text/javascript'>


Please move this to a js file, probably can combine it with the snippet above.

theacodes · 2016-10-06T16:44:26Z

appengine/standard/firebase/firetactoe/firetactoe.py

+    game = GameFromRequest(self.request).get_game()
+    user = users.get_current_user()
+    if game and user:
+      FireChannel(fire_url).send_message(u_id=user.user_id() + game.key().id_or_name(), 


Hanging indents are really unreadable, please line break after send_message(.

theacodes · 2016-10-06T16:45:41Z

appengine/standard/firebase/firetactoe/firetactoe.py

+				method_type = urlfetch.PATCH
+			# always using patch in this example for writes
+			# because it updates or creates but never overwrites
+			result = urlfetch.fetch(


urlfetch is rough, why not use requests? We have a sample here.

theacodes · 2016-10-06T16:46:22Z

appengine/standard/firebase/firetactoe/firetactoe.py

+			json_data = simplejson.loads(result.content)
+			logging.info(json_data.get("message"))
+			return result.content
+		except urlfetch.Error:


In samples, it's better to let the exception raise all the way up so that the user can see the stacktrace. This helps us figure out what's wrong if they report issues.

theacodes · 2016-10-06T16:46:44Z

appengine/standard/firebase/firetactoe/firetactoe.py

+          game.userO = user
+          game.put()
+
+      game_link = 'http://localhost:8080/?g=' + game_key


Hardcoded URL?

theacodes · 2016-10-06T16:47:04Z

appengine/standard/firebase/firetactoe/firetactoe.py

+    ('/move', MovePage)], debug=True)
+
+
+def main():


This isn't needed for app engine.

jerjou · 2016-10-06T20:55:42Z

Oh whoops. I'm totally working on a refactor of this -_-;
Opened up #560 with the refactor. It addresses most of the comments from this PR.
How would you like to proceed, @mogar1980?

firetactoe pull request

befd3c5

googlebot added the cla: no This human has *not* signed the Contributor License Agreement. label Oct 5, 2016

waprin reviewed Oct 5, 2016

View reviewed changes

googlebot added cla: yes This human has signed the Contributor License Agreement. and removed cla: no This human has *not* signed the Contributor License Agreement. labels Oct 5, 2016

waprin reviewed Oct 5, 2016

View reviewed changes

theacodes self-assigned this Oct 6, 2016

theacodes suggested changes Oct 6, 2016

View reviewed changes

jerjou mentioned this pull request Oct 6, 2016

Add firebase-based tic-tac-toe sample. #560

Merged

theacodes closed this Oct 10, 2016

Conversation

mogar1980 commented Oct 5, 2016

Uh oh!

googlebot commented Oct 5, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mogar1980 commented Oct 5, 2016

verify. Thanks.

Uh oh!

googlebot commented Oct 5, 2016

Uh oh!

waprin commented Oct 5, 2016

Uh oh!

mogar1980 commented Oct 5, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

waprin commented Oct 5, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mogar1980 commented Oct 5, 2016

@waprin commented on this pull request.

Uh oh!

theacodes commented Oct 6, 2016

Uh oh!

theacodes left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jerjou commented Oct 6, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants