initial development #3

sqweelygig · 2017-01-25T12:31:23Z

created an abstract class for polling an api
closes Should work(!) #1

* created an abstract class for polling an api

Page- · 2017-02-01T21:56:29Z

src/abstract-api-adapter.coffee

+		super
+		@interval = 1000
+		@lastReport = null
+		@clearToPoll = true


What does clearToPoll control? The name isn't obvious to me and I'm wondering if there's a better alternative

Page- · 2017-02-01T21:58:24Z

src/abstract-api-adapter.coffee

+	processDone: (error, response, report, done) =>
+		@clearToPoll = true
+		@report report
+		if done?


This can be done?(error, response) which will specifically check if done is a function rather than just nullish

* made method that deals with top level methodology * relies on implementing classes to perform a couple of methods

sqweelygig · 2017-02-04T17:25:05Z

closes #2
closes #4
(only done on this PR because the code I've just done is dependant on previous code)

pimterry · 2017-02-06T19:40:14Z

package.json

-  "description": "A base adapter for Hubot",
-  "main": "./src/base-adapter.coffee",
+  "name": "hubot-abstract-api-adapter",
+  "version": "0.0.2",


It's a fairly minor point in context, but just for good practice: this is certainly a large enough change to bump to warrant a minor bump.

pimterry · 2017-02-06T19:43:06Z

src/abstract-api-adapter.coffee

+class AbstractAPIAdapter extends Adapter
+	constructor: ->
+		super
+		@interval = 1000


Could be a constant (assuming external clients aren't supposed to change this)

Inherited classes and situation may dictate changes to this. eg if a server is refusing due to usage limits or high load we may choose to increase our poll interval.

Ok, if this is intended I'd make this more explicit - e.g. a constructor argument that defaults to 1000.

pimterry · 2017-02-06T19:45:54Z

src/abstract-api-adapter.coffee

+		@interval = 1000
+		@lastReport = null
+		@allowedToPoll = true
+		throw new TypeError('Abstract class')


What? How are subclasses supposed to be able to instantiate this? http://stackoverflow.com/a/30560792/68051 has some good suggested approaches that'll do what you're looking for.

pimterry · 2017-02-06T19:47:59Z

src/abstract-api-adapter.coffee

+		super
+		@interval = 1000
+		@lastReport = null
+		@allowedToPoll = true


We could make the usage of this clearer by flipping the boolean, and renaming it to pollInProgress.

pimterry · 2017-02-06T19:50:21Z

src/abstract-api-adapter.coffee

+				if response?.statusCode in [429, 503]
+					@report { error: error, html: response?.statusCode, url: options.url }
+					# Retry if the error code indicates temporary outage
+					@getUntil options, filter, each, done


You've flipped the order of filter/each here.

pimterry · 2017-02-06T20:07:15Z

src/abstract-api-adapter.coffee

+			if not error and headers.statusCode is 200
+				parseResponse(JSON.parse(body), callback)
+			else
+				callback(error ? headers, null)


Calling with error makes sense, but headers is a weird fallback. It's be good behaviour for this to be an actual Error instance. Something like: new Error("Received #{headers.statusCode} response: #{body}") would be better.

pimterry · 2017-02-06T20:11:32Z

src/abstract-api-adapter.coffee

+		# Format taken from https://github.com/github/hubot/blob/master/docs/scripting.md#making-http-calls
+		requestObject.post(JSON.stringify(requestObject.payload)) (error, headers, body) ->
+			if not error and headers.statusCode is 200
+				parseResponse(JSON.parse(body), callback)


I'd wrap the JSON.parse in a try/catch, and then callback with an error complaining about the body if it fails. We're not checking the headers here to check we're getting JSON atm (which isn't 'perfect', but parsing optimistically is fine for this), and eventually we'll definitely hit an issue somewhere here somebody doesn't give us JSON back (some HTML error page or something), which currently will silently kill this method - it'll never call the callback at all.

pimterry · 2017-02-06T20:13:24Z

src/abstract-api-adapter.coffee

+	* @param {object} ids to use.  ids = {user, flow, thread?}
+	* @param {function} function to receive (error, new ids object).  ids = {thread, comment?}
+	###
+	postUsing: (text, ids, callback) =>


Is there a reason we're not using promises here, and for done in getUntil/processDone? It'd make this code and code using this tidier and more reliable (done right, you should get automatic error -> promise error propagation, rather than having to ensure we're guaranteed to call the callback in every case)

Does my not knowing promises yet count as a reason?

It counts as an extra good reason to do it, since they're a core part of lot of the rest of the codebase, and this will be a very convenient place to learn them :D. Jake Archibald has a good intro: https://developers.google.com/web/fundamentals/getting-started/primers/promises.

To be clearer on the end goal: it'd be nice if postUsing returned a promise, and didn't take callback, and if getUntil and processDone both returned promises, and didn't take done. Ping me if you've got any questions or if you need a hand, happy to help.

pimterry · 2017-02-06T20:14:29Z

src/abstract-api-adapter.coffee

+		requestObject = @robot.http(requestDetails.url)
+		requestObject.header('Accept', 'application/json')
+		for key, value of requestObject.headers
+			requestObject.header(key, value)


...what are these two lines doing? This seems very strange. Is this supposed to be looping over requestDetails instead?

pimterry · 2017-02-06T20:18:14Z

src/abstract-api-adapter.coffee

+						return
+				if @extractNext responseObject
+					options.url = @extractNext responseObject
+					@getUntil options, filter, each, done


Flipped filter/each here too.

This does suggest we haven't really tested this code... Can we have some automated tests for this somewhere to catch these issues? Here if that's easy, or an integration test in an example extending class somewhere else would be great.

sqweelygig · 2017-02-07T09:43:22Z

It's true, this PR is relatively untested. It originated from a time before I'd started implementing alpha branches, so at the time couldn't test until implemented, and couldn't develop from this foundation until accepted. Since then I'm trialling a slightly different branch convention that should allow me to conduct dependant development while a PR is in progress.

sqweelygig · 2017-02-07T12:15:55Z

closes #4

lurch · 2017-02-09T16:55:40Z

Do I need to ignore this as there's no mention of marshmallows? ;-)

lurch · 2017-02-09T17:06:13Z

package.json

-	"hubot-mock-adapter": "^1.0.0",
-	"hubot-slack": "^4.3.1",
-	"resin-lint": "^1.4.0"
+    "hubot": "^2.19.0",


Why is this in devDependencies rather than dependencies?

lurch · 2017-02-09T17:08:57Z

src/abstract-api-adapter.coffee

+	###*
+	* Given a set of ids make best effort to publish the text and pass on the published ids
+	* @param {string} text to publish
+	* @param {object} ids to use.  ids = {user, flow, thread?}


Do the mentions of flow and thread mean that this is specific to Flowdock, and maybe as not as "abstract" as you intended?

Every one of our chat systems uses a different terminology to mean the same thing. It's muddling me up so much that I've got a chart of the equivalences in my exercise book.
This said, the most common comms tool in resin is Flowdock, so where I've a choice I've used Flowdock's nouns.

lurch · 2017-02-09T17:10:55Z

src/abstract-api-adapter.coffee

+	* @param {string} text to publish
+	* @param {object} ids to use.  ids = {user, flow, thread?}
+	###
+	postUsing: (text, ids) -> new Promise (resolve, reject) =>


I know nothing about writing hubot scripts, but should there also be a getUsing?

Nah. This is actually nothing to do with hubot's scripts and is a utility function for without context posting. Each adapter has a way of getting without context.

initial development

9809a44

* created an abstract class for polling an api

sqweelygig requested a review from Page- January 25, 2017 12:35

sqweelygig self-assigned this Jan 25, 2017

Page- reviewed Feb 1, 2017

View reviewed changes

review feedback

adbed13

sqweelygig requested review from lurch, pimterry and CameronDiver February 4, 2017 15:03

postUsing skeleton

d8e200a

* made method that deals with top level methodology * relies on implementing classes to perform a couple of methods

Merge remote-tracking branch 'remotes/origin/1-initial' into alpha

158c5e8

pimterry suggested changes Feb 6, 2017

View reviewed changes

Andrew Lucas added 2 commits February 7, 2017 09:04

0.1.0

5861656

review feedback

81878f7

review feedback

6b59a67

lurch reviewed Feb 9, 2017

View reviewed changes

review feedback

342a921

sqweelygig force-pushed the 1-initial branch from 6c7d3ec to 342a921 Compare February 9, 2017 18:07

Andrew Lucas added 2 commits February 10, 2017 12:27

updated docs

0e7a462

Merge remote-tracking branch 'remotes/origin/1-initial' into 1-initial

7fbdf3b

sqweelygig requested a review from hedss February 20, 2017 09:19

sqweelygig closed this Feb 23, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

initial development #3

initial development #3

sqweelygig commented Jan 25, 2017

Page- Feb 1, 2017

Page- Feb 1, 2017

sqweelygig commented Feb 4, 2017

pimterry Feb 6, 2017

pimterry Feb 6, 2017

sqweelygig Feb 7, 2017

pimterry Feb 7, 2017 •

edited

Loading

pimterry Feb 6, 2017

pimterry Feb 6, 2017

pimterry Feb 6, 2017

pimterry Feb 6, 2017

pimterry Feb 6, 2017

pimterry Feb 6, 2017

sqweelygig Feb 7, 2017

pimterry Feb 7, 2017

pimterry Feb 6, 2017

pimterry Feb 6, 2017

sqweelygig commented Feb 7, 2017

sqweelygig commented Feb 7, 2017

lurch commented Feb 9, 2017

lurch Feb 9, 2017

lurch Feb 9, 2017

sqweelygig Feb 9, 2017

lurch Feb 9, 2017

sqweelygig Feb 9, 2017 •

edited

Loading

initial development #3

initial development #3

Conversation

sqweelygig commented Jan 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sqweelygig commented Feb 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pimterry Feb 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sqweelygig commented Feb 7, 2017

sqweelygig commented Feb 7, 2017

lurch commented Feb 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sqweelygig Feb 9, 2017 • edited Loading

Choose a reason for hiding this comment

pimterry Feb 7, 2017 •

edited

Loading

sqweelygig Feb 9, 2017 •

edited

Loading