Initial WIP for procbots. #5

hedss · 2017-01-27T13:01:40Z

See README.md for details.

hedss · 2017-01-27T13:10:31Z

All,
The code is not yet 'complete' in that, whilst functionally it works correctly on repositories, the ProcBots themselves need logging/alerting adding, @brownjohnf has very kindly offered to help me with this, so the plan is to hand this over to him. ;-) So whilst this is indeed a review, it's a tentative review because of this and especially because...

This is my first major foray into TypeScript:
@emirotin I think you were working on a coding standard, is this ready yet?
@pimterry You have probably come across this, what's the best format for type declaration for things such as wrapping modules in Promises (specifically Bluebird), without throwing this info away?

Source files in lib, built files (including maps) in build.

For clarity, I've attempted to defined partial interfaces for those components used heavily (such as the github NPM module, which doesn't have interfaces in its @types definition) and generally for anything created by a class.

I've built a gulpfile configuration that seems to work. I've come across a few oddities that need specifically adding to allow mapping to work in VS Code. These are documented in the relevant places.

I've also carried out a possible JSDoc implementation for the Worker class in ProcBots.

I'd appreciate any thoughts on any of this. Cheers! :)

pimterry

I've added a whole bunch of TypeScript points I spotted flicking through. I haven't done a detailed review of the core bot logic, since there's loads of it, I don't have much context, and I'm assuming somebody else does, but I can take a closer look if that would be useful.

You have probably come across this, what's the best format for type declaration for things such as wrapping modules in Promises (specifically Bluebird), without throwing this info away?

I think you're talking about keeping type definitions for a module when it's passed through promisifyAll? That's tricky, sadly. I tend to (in approx this order of preference):

Find a pre-promisified equivalent version of the module that already has type definitions (mz would cover a good few of your cases here).
Promisify individual methods instead if I only need one or two, instead of using promisifyAll, since that should be correctly fully typed by Bluebird for most normal cases.
Any type the whole thing and move on with my life.
Put together my own type definitions, if it's a module I'm using a lot (easier than it sounds, since it's a fairly clearcut transform of the real definitions, but still a bit annoying)

pimterry · 2017-01-27T14:52:21Z

lib/app.ts

+*/
+
+// Import most things with types if possible.
+import Opts = require('node-getopt');


Node-getopt does have published type definitions, and you've actually got them installed. Is there a reason this isn't a proper import?

Pure forgetfulness that I'd included them, had meant to change it prior to release.

Actually, I now remember why. It's because the node-getopt declarations declare using the export = format and not a namespace. Therefore attempting to 'import ... from 'node-getopt'` fails, as it doesn't resolve to a module. Keeping as-is.

Ah, makes sense. I think maybe I'd parsed this as var Opts = require('node-getopt') (or let or const), which is the case we need to avoid. import X = ... I'm fine with, if that's what works.

pimterry · 2017-01-27T14:53:55Z

lib/app.ts

+// Run the hook listener now, and for each bot we've been asked to listen
+// to, wait for events. When we get an event, check against those bots
+// registered, and send filtered messages on to them for action.
+app.post('/webhooks', (req: any, res: any) => {


You shouldn't need to any type these, if you just leave the types off they should get inferred as proper request/response types anyway.

I've left some of these in for commenting on; I'm more than happy to rely on inferring all types, though I did wonder if it's sometimes clearer to ensure. In the case of any, I guess it really doesn't help.

pimterry · 2017-01-27T14:54:53Z

lib/app.ts

+	res.sendStatus(200);
+
+	// Go through all registered bots, and send them any appropriate hook.
+	_.forEach(botRegistry, (bot: GithubBot.GithubBot) => {


Don't need the type definition here either, this should be inferred too (although in this case it'll be the same result, so there is no downside to being explicit, if that's what you want)

+1 for the explicitness IMO, I mean that's why we're using typescript in the first place. The types don't exist at interpreter level which means that they are only there for the programmer and compiler, so whilst it can be more helpful to let it infer, I think in the interest of understanding being explicit is better.

You can still get the info by mousing over in VS Code, if that's what you're looking for.

Regardless though, sure, if people are getting value from this being more explicit then that's good enough for me 👍

I'd left it to ensure that it was realised that it's firing on the inherited class. But actually, now I look at it again, the type of botRegistry isn't too far up and is explicit, so I think I'll remove it.

Relying on somebody using a specific editor feels off to me (ofc there'll be packages for other editors, but you get what I'm saying). Also if the type inference is anything like Haskell's, which makes sense, the inferred types are the most generic that they possibly can be to still fit the domain. This may not be as helpful as hand crafted types in some situations.

Hmm. On the fence now then. :) The C++ lad in me might just leave it in then...

Ok, it sounds like for code like this, if you like it, you should leave them in.

For things like var x: X = new X(), where the type is clear and very close to the definition, definitely drop it.

In the middle ground, pick whichever option is closer. Here though I think you're right @CameronDiver that it's not instantly obvious what the type is - it's a couple of blocks back and you can't immediately see it at a glance - so this seems reasonably useful to me.

pimterry · 2017-01-27T15:00:04Z

lib/githubbot.ts

+}
+
+// A GithubActionMethod is the method that will be used to process an event.
+export type GithubActionMethod = <T>(action: GithubAction, data: T) => Promise<void>;


Purely style, but personally when I have a lot of interfaces to define like this big chunk here, I'd normally pull out them out into a separate file, and then import the interfaces into this implementing module.

Makes this file much easier to dive into, and also has the nice advantage that elsewhere people can just depend on the interface, and you can change the implementation code with complete impunity, as long as the interface stays in the same place.

Agreed, keep the model and the logic seperate as much as possible.

See comment below; essentially exposing all interfaces, even when some are only going to be used by a specific class 'seems' wrong.

I think for this bit, they should indeed definitely all be public, in a separate file. They're all already exported publically anyway, and you'll need them in other downstream Github bots, so they're a meaningful part of the interface.

pimterry · 2017-01-27T15:08:52Z

tools/initRepo/lib/initRepo.ts

+*/
+
+// Import most things with types if possible.
+import Opts = require('node-getopt');


Same point about node-getopt types

As with comment above, this is required.

pimterry · 2017-01-27T16:05:09Z

lib/githubbot.ts

+    // However, 'baked in' registrations are not available for deregistration.
+    constructor(integration: number) {
+        super();
+        this._botname = 'GithubBot';


The setting of _botname feels like a strangely secret interface of ProcBot. Why not make it an optional constructor argument, so you can just pass it to super()?

I originally had... and I'm not quite sure why it's changed. I suspect during other changes, and I just didn't revert this.

pimterry · 2017-01-27T16:13:08Z

lib/versionbot.ts

+                if ((commit.committer.name === process.env.VERSIONBOT_NAME) &&
+                    _.find(files, (file: GithubBot.CommitFile) => {
+                        return file.filename === 'CHANGELOG.md';
+                    })) {


The indentation here plus the double indentation of the block below and the callback inline in the test makes this really really confusing to read, for me at least.

Shall change.

pimterry · 2017-01-27T16:16:07Z

lib/versionbot.ts

+                            repo: name,
+                            number: pr.number,
+                            commit_title: `Auto-merge for PR ${pr.number} via Versionbot`
+                        }, 3).then((mergedData:GithubBot. Merge) => {


This triple nested level of promises in this fairly hefty function is all a bit hard to keep track of. Can we break it up a bit?

You could flatten easily by returning immediately from the gitCall for getCommit above, and this one here is trickier, but you could pull this chunk out nicely into a separate merge function:

if (committer === versionbot && ChangelogWasCommitted) { return merge(pr, owner, name, headCommit); }

Yep, in fact it's been my intention to do this, but I really wanted some initial comments on this before the weekend. :)

pimterry · 2017-01-27T16:18:01Z

lib/versionbot.ts

+                            repo: name,
+                            number: pr.number,
+                            commit_title: `Auto-merge for PR ${pr.number} via Versionbot`
+                        }, 3).then((mergedData:GithubBot. Merge) => {


Bonus nit: this line has a space in the argument's type? I'm surprised that compiles, strange.

That's really odd. gulp does indeed compile it.

It looks like valid syntax to me. Whitespace shouldn't make a difference.

pimterry · 2017-01-27T16:24:00Z

lib/versionbot.ts

+            // 'approved' variable, therefore it implicitly believes it can only ever be 'false'
+            // and barfs on the 'if (approved === true)' line claiming 'true' cannot be 'false'.
+            // Not great. Still, easy to use a straight for() loop.
+            approved = false;


You've got approved = false here twice.

Also, I think you can get out of the issues you explain in the comment above if you assert approved as boolean in the if condition. Not sure if it's significantly better, but an interesting option:

if (approved === true) { // Doesn't compile, === can't be applied to false and true } if (<boolean> approved === true) { // Works fine }

Ah, explicit casting! Cool, thanks.

CameronDiver · 2017-01-27T17:00:56Z

tsconfig.json

@@ -0,0 +1,12 @@
+{
+	"compilerOptions": {
+		"module": "commonjs",


We probably want declarations: true here so that typescript will generate type information for anyone using this module.

emirotin · 2017-01-31T07:53:44Z

I didn't do anything for the TS coding standard TBH. If @pimterry can recommend any popular linting preset I'd be fine with it without much bikeshedding.

* Separate interfaces into separate declaration files * Move Worker class into own source file * Use callback method to remove Worker from context map in child, rather than Worker class itself (cleaner) * Correct pointer to declaration files in package.json (now correctly points to `build`) * Add `tslint` gulp task, to ensure code is linted, add custom `tslint.json` that expands upon the default rules * Refactor code after initial linting * Include typings for a few libraries that were not typed * Add 'copydecs' task to copy explicit declaration files to `build` directory for interfaces * Github API is now imported as correct types * Untyped methods are now explicitly Promisified with a function type * Correct initRepo package.json * Ensure all libraries adhere to `tslint `import rules * Remove unused imports and variables * Add checks for unused variables and expressions to `tsconfig.json` * Correct callback for setTimeout in GithubBot * Split up unwieldy methods in VersionBot * Remove unused parentMap structure in Worker * Correct some typos

hedss · 2017-01-31T15:49:36Z

@emirotin No worries, we've had some discussion and I'm going to put something together in the next day as a spec. we can all discuss. :)

…instead of setting it in each derived child. Also remove implicitly typed variable.

* Update `gulpfile.js` to remove now unrequired sourcemap option * Add `typings` directory, add typing for used `temp` functionality and include the `typings` directory in the `tsconfig.json` * Update spacing correctly in files * Ensure Promisified `temp` requirements use the typings by importing the `temp` module as normal

CameronDiver · 2017-02-02T12:26:06Z

tslint.json

+{
+    "extends": "tslint:recommended",
+    "rules": {
+        "indent": "tabs",


Didn't we agree on spaces?

We did; I'm also going to be dumping this file once I've got resin-tslint done, so tbh, I'm not hugely bothered. Shall change.

Ah ok - makes sense.

pimterry · 2017-02-02T12:40:54Z

tsconfig.json

+        "strictNullChecks": true,
+        "sourceMap": true,
+        "target": "es6",
+        "baseUrl": "./typings",


I'm surprised by baseUrl. I've never seen it before, and the docs say it exists mainly for AMD projects.

Is this so that require('temp') finds typings/temp.d.ts? In that case, I think you just need to ensure that the file is already included in the build process - i.e. add typings/*.d.ts to includes here or your gulp.files() argument above. The module shouldn't need to resolved correctly from its name because it's an ambient module declaration - it will define the module name you can load it by explicitly by itself, independent of its location, as long as it's in the build.

I followed from this thread, as I was originally attempting to use typeRoots.

Adding it to the gulp file appears to cause Visual Code to complain as it doesn't find them. I'll use includes if that's better than baseUrl.

pimterry · 2017-02-02T12:49:51Z

lib/versionbot.ts

-const tempCleanup = Promise.promisify(require('temp').cleanup);
+const fsReadFile = Promise.promisify(FS.readFile);
+const tempMkdir = Promise.promisify(mkdir);
+const tempCleanup = Promise.promisify(track);


You can promisify track if you like, but looking at the docs it doesn't take a callback or return anything at all, so I probably wouldn't 😄.

That's a good point.

pimterry · 2017-02-02T12:53:06Z

typings/temp.d.ts

@@ -0,0 +1,4 @@
+declare module 'temp' {
+    function track(): void;
+    function mkdir(prefix: string): (err: Error, dirPath: string) => void;


mkdir returns a callback? Seems unlikely - this should be the 2nd argument, and mkdir probably returns void.

This suggests the type you're getting when you promisify this isn't right either atm. Looks like the only reason you don't hit an implicit any is because the only place you call it you then immediately specify the next promise argument type.

Promisify does return the correct promisified type if the function you pass to promisify takes a callback as the last argument, but if it doesn't then it'll return a plain Function (i.e. callable, but takes and returns any). The types are little hacky but kind of interesting, if you're into that sort of thing: https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/bluebird/index.d.ts#L390-L403. It means it works most of the time, but it's not totally foolproof. Implicit any should catch any substantial issues generally though.

pimterry

Some last small comments, but once those are done this looks good to me.

* Modify `tsconfig.json` to specifically `include` correct typings and source * Ensure `gulpfile.js` uses specified files from `tsconfig.json` * Update `tslint.json`

CameronDiver

LGTM :)

Initial WIP for procbots.

e947d0a

See README.md for details.

hedss requested review from Page-, emirotin, brownjohnf, pimterry, lekkas and CameronDiver January 27, 2017 13:10

hedss requested a review from craigmulligan January 27, 2017 13:18

pimterry suggested changes Jan 27, 2017

View reviewed changes

CameronDiver reviewed Jan 27, 2017

View reviewed changes

hedss added the flow/3/in-progress label Jan 31, 2017

Ensure ProcBot type has constructor that is passed an optional name, …

476dcf8

…instead of setting it in each derived child. Also remove implicitly typed variable.

hedss added flow/4/in-review and removed flow/3/in-progress labels Jan 31, 2017

hedss added 2 commits January 31, 2017 17:28

Remove comments from tsconfig.json.

19dc480

CameronDiver reviewed Feb 2, 2017

View reviewed changes

pimterry reviewed Feb 2, 2017

View reviewed changes

pimterry suggested changes Feb 2, 2017

View reviewed changes

hedss added 3 commits February 2, 2017 13:18

Update post-review.

486fe15

* Modify `tsconfig.json` to specifically `include` correct typings and source * Ensure `gulpfile.js` uses specified files from `tsconfig.json` * Update `tslint.json`

Fix spacing error and ordering issue.

e01f46e

Correct typings for the mkdir function.

bc63655

CameronDiver approved these changes Feb 2, 2017

View reviewed changes

pimterry approved these changes Feb 2, 2017

View reviewed changes

hedss merged commit a7d62eb into master Feb 2, 2017

hedss deleted the initial-work branch February 2, 2017 17:29

lekkas mentioned this pull request Feb 3, 2017

There is no current WIP for initial version #1

Closed

pimterry mentioned this pull request Feb 22, 2017

Initial code PR balena-io-modules/resin-docker-build#2

Merged

hedss added this to Closed in ProcBot Work Mar 22, 2017

hedss removed the flow/4/in-review label Mar 22, 2017

lurch mentioned this pull request Jun 23, 2017

ProcBots: Move from spaces in source to tabs #165

Closed

Initial WIP for procbots. #5

Initial WIP for procbots. #5

Conversation

hedss commented Jan 27, 2017

hedss commented Jan 27, 2017 • edited

pimterry left a 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

pimterry Jan 30, 2017 • edited

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

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

pimterry Jan 27, 2017 • edited

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

emirotin commented Jan 31, 2017

hedss commented Jan 31, 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

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

pimterry left a comment

Choose a reason for hiding this comment

CameronDiver left a comment

Choose a reason for hiding this comment

hedss commented Jan 27, 2017 •

edited

pimterry Jan 30, 2017 •

edited

pimterry Jan 27, 2017 •

edited