Review feedback #7

bajtos · 2019-08-20T08:51:51Z

I quickly skimmed through the code base in https://github.com/markdirish/loopback-connector-ibmi, the code looks reasonably. I have few suggestions & discussion points. /cc @markdirish

(1)
The code handling transactions in executeSQL does not look right to me. If I understand it correctly, then it's opening a new transaction for every SQL command executed - that's not what we want. Instead, the code should execute the given SQL command within the given transaction that has been opened previously, see how our PostgreSQL connector handles that here.

(2)
The connector should run tests for operation hooks in addition to "common.batch" and "include.test".

(3)
Ideally, the connector should run tests from both v3 and v4 versions of juggler. See loopbackio/loopback-connector-mongodb#519 for inspiration.

(4)
I noticed that eslint is configured with airbnb style, it would be nice to use LoopBack presets instead.

(5)
It's not clear how to run tests locally and AFAICT, there is no CI configured. How are we going to verify pull requests? Will each change require a manual run of the tests? It would be great if there was a way how to run DB2 on IBMi via Docker, that would simplify dev setup and allow us to run tests on Travis CI too.

(6)
There are two repositories ATM: https://github.com/markdirish/loopback-connector-ibmi and https://github.com/strongloop/loopback-connector-ibmi, these two repos seems to be out of sync. We should pick one that will server as the single source of truth, bring it up to date with the latest working version, and update URLs in package.json so that they all point to the same repository.

ATM, the URLs are mixed:

https://github.com/strongloop/loopback-connector-ibmi/blob/6103967a0e4cc7eaf1b150c41b19987ad0b3bc4b/package.json#L45-L52

  "repository": {
    "type": "git",
    "url": "git://github.com/https://github.com/markdirish/loopback-connector-ibmi.git"
  },
  "homepage": "https://github.com/markdirish/loopback-connector-ibmi",
  "bugs": {
    "url": "https://github.com/strongloop/loopback-connector-db2iseries/issues"
  },

(7)
Last but not least, the license field in package.json says Artistic-2.0. AFAIK, IBM prefers Apache 2.0 or MIT for open source projects. We are using MIT in StrongLoop repos, I think we should use the same license for this connector too.

The text was updated successfully, but these errors were encountered:

markdirish · 2019-08-20T13:24:59Z

@bajtos Thanks for the review, I will work on these today. Some quick thoughts on non-code issues:

I will take a look at how PostgreSQL does transactions

2 and 3: I will look into the tests today to make sure everything works right now (unlikely, but then I can fix them)

AirBnB is the lint we use on the IBM i OSS team, so when I forked the repo I probably set it to that by default. I will adjust to LoopBack presets.
Unfortunately, there is no way to create a docker image of IBM i (licenses and all that). I do have access to a machine that can serve as the CI testing platform, I have yet to set it up but I can look into it today. The machine should always be on and available on the internet. Tests against the database can also be run remotely, but I think most people will want to run everything directly on IBM i, so the CI tests should mimic that as much as possible.
My personal loopback-connector-ibmi should be ignored, support of the connector has been moved to StrongLoop. I will work on fixing the package.json issues, but will probably keep my fork open for development and PR staging.
I'm not well-versed on how the licenses work, but I initially forked and based much of my work off of loopback-connector-db2iseries, which uses the Artistic 2.0 license.

dhmlau · 2019-08-20T14:40:07Z

I think some of our connectors are using Artistic 2.0 license.

@markdirish, I just realized there's no CLA or DCO set up in this repo (unless i'm missing something). We're trying to move from CLA to DCO (still in process!), so I'd recommend you to use DCO as the contribution method. If you agree, I can enable it for you. I think it needs admin for the github org to do that.

markdirish · 2019-08-20T14:41:47Z

Our team is also using DCO, so it would be great to set up here as well!

dhmlau · 2019-08-20T14:47:02Z

It's enabled now! :)

You might want to update the CONTRIBUTING content: https://github.com/strongloop/loopback-connector-ibmi/blob/master/CONTRIBUTING.md.

For your reference, here is the CONTRIBUTING.md in one of our DCO-enabled repo: https://github.com/strongloop/loopback.io-search/blob/master/CONTRIBUTING.md

kadler · 2019-08-20T15:27:48Z

It's not clear how to run tests locally and AFAICT, there is no CI configured. How are we going to verify pull requests? Will each change require a manual run of the tests? It would be great if there was a way how to run DB2 on IBMi via Docker, that would simplify dev setup and allow us to run tests on Travis CI too.

IBM i is an entire OS (with integrated Db2 DBMS), so it can't really be run in Docker. There are finally ways to run IBM i in IBM Cloud, SkyTap, and Google Cloud. They're all pretty early on and there currently isn't any commercial CI system built around them, either.

Our team is actively working on enabling some sort of IBM i CI system for OSS, though.

markdirish · 2019-09-06T02:43:57Z

@bajtos ,

I have been working on this a lot lately, and have my connector passing 100% of the tests in loopback-datasource-juggler v. 2.43.0.

I notice that There is a much newer version, 4.12.1. Should I target the tests in that version instead? Similarly, I have an outdated version of loopback-connector. Should I upgrade that to v 4.8.0 as well? And will there be any issue that loopback-connector 4.8.0 has a dev dependency of loopback-datasource-juggler v.3.32.0, and I would be using a newer version?

I have tinkered around with the new versions, but there are things that don't seem to be working quite right. If you let me know what dependency versions I should target, I can analyze further and post the issues I'm having.

Thanks!

markdirish · 2019-09-06T13:57:09Z

I have pushed my latest changes to https://github.com/markdirish/loopback-connector-ibmi/tree/master

I updated the dependencies for loopback-connector and loopback-datasource-juggler to the latest versions. I have about ~60 failing tests that all seem to have a similar root issue that I'm not sure how to fix, and I don't know if its a silent version mismatch or what:

Consistently, when a test calls Model.create and only passes in a function, I am getting a blank object back (doesn't have the ID or any attributes). For instance:

https://github.com/strongloop/loopback-datasource-juggler/blob/ad1777fcd3d1dd120634b7c140c3313559c93965/test/relations.test.js#L78-L85

When book is returned, it is blank. When c is built, bookId is undefined. I have checked my connector, and it appears that getInsertedId is implemented correctly. and in the datasource-juggler dao.js, the correct id is returned at this point:

https://github.com/strongloop/loopback-datasource-juggler/blob/master/lib/dao.js#L375-L383

However, by the time gets to line 424, the object appears to be the blank object that is returned to the test.

Is this an issue with my connector, with these tests, or with the datasource-juggler?

Thanks!

markdirish · 2019-09-06T16:21:51Z

Ok, I have resolved my id issue (seems like it was defaulting to 0, when it should have been defaulting to 1, and was failing silently somewhere). Now have 658 passing tests and 11 failing. Will try to run tests against v3 and v4 as you mention above.

markdirish · 2019-09-13T17:06:39Z

@bajtos , I think I am nearing completion of getting loopback-connector-ibmi working 100%. Running all the operation hooks tests and tests for juggler v3 and juggler v4, I have 1366 passing, 50 failing. I am looking for help on debugging some of my issues. (Of course, the fact that this runs on IBM i and there are no publicly available instances for you to confirm these issues will be difficult, but hopefully we get a box that can be used for CI soon).

All of the code I am referencing can be found at: https://github.com/markdirish/loopback-connector-ibmi/tree/master/. I copied the test structure from loopback-connector-mysql and loopback-connector-mongodb

The biggest head-scratcher for me is how DataSources are created for testing with the getSchema() function, which calls the DataSource constructor. For a few tests, such as the Persistence hooks tests, it seems as though there are issues with a new data source when created at: https://github.com/markdirish/loopback-connector-ibmi/blob/07565ff074a7df2a0d3e1cafb0f2a264ee8b881b/deps/juggler-v3/test.js#L34-L36 . However, it is being created with a constructor synchronously. But it isn't waiting for the connector's init function to return, so it starts to try to make database queries, but the connection pool hasn't been defined yet. Looking through the datasource-juggler code, it looks like it initializes here, but for whatever reason it seems as though it is trying to execute SQL on the connector before the postInit callback is called.
In the replaceAttributes tests, the getSchema() function is causing an UnhandledPromiseRejection Warning. The offending line is here: https://github.com/strongloop/loopback-datasource-juggler/blob/ad1777fcd3d1dd120634b7c140c3313559c93965/test/manipulation.test.js#L1358 . When I move it inside the before() function, it works as expected (and this is the pattern that most other tests use). Does this sound right, and should I make a PR?
The only tests that seem to be based on a connector issue itself are related to default scope. A sample stack trace from a failing test is like:

Uncaught TypeError: Cannot read property 'name' of null
      at /home/mirish/lb/loopback-connector-ibmi/deps/juggler-v3/node_modules/loopback-datasource-juggler/test/default-scope.test.js:229:14
      at /home/mirish/lb/loopback-connector-ibmi/deps/juggler-v3/node_modules/loopback-datasource-juggler/lib/dao.js:1798:62
      at /home/mirish/lb/loopback-connector-ibmi/deps/juggler-v3/node_modules/loopback-datasource-juggler/lib/dao.js:1734:9
      at /home/mirish/lb/loopback-connector-ibmi/deps/juggler-v3/node_modules/async/dist/async.js:1140:9
      at /home/mirish/lb/loopback-connector-ibmi/deps/juggler-v3/node_modules/async/dist/async.js:473:16
      at eachOfArrayLike (deps/juggler-v3/node_modules/async/dist/async.js:1057:9)
      at eachOf (deps/juggler-v3/node_modules/async/dist/async.js:1117:5)
      at _asyncMap (deps/juggler-v3/node_modules/async/dist/async.js:1133:5)
      at Object.map (deps/juggler-v3/node_modules/async/dist/async.js:1122:16)
      at allCb (deps/juggler-v3/node_modules/loopback-datasource-juggler/lib/dao.js:1645:13)
      at /home/mirish/lb/loopback-connector-ibmi/node_modules/loopback-connector/lib/sql.js:1457:7
      at /home/mirish/lb/loopback-connector-ibmi/deps/juggler-v3/node_modules/loopback-datasource-juggler/lib/observer.js:250:22
      at doNotify (deps/juggler-v3/node_modules/loopback-datasource-juggler/lib/observer.js:155:49)
      at IBMiConnector.ObserverMixin._notifyBaseObservers (deps/juggler-v3/node_modules/loopback-datasource-juggler/lib/observer.js:178:5)
      at IBMiConnector.ObserverMixin.notifyObserversOf (deps/juggler-v3/node_modules/loopback-datasource-juggler/lib/observer.js:153:8)
      at cbForWork (deps/juggler-v3/node_modules/loopback-datasource-juggler/lib/observer.js:240:14)
      at /home/mirish/lb/loopback-connector-ibmi/node_modules/loopback-connector/lib/sql.js:648:9
      at process.nextTick (/home/mirish/odbc/node-odbc/lib/Pool.js:142:9)
      at process._tickCallback (internal/process/next_tick.js:61:11)

Not sure if you have any clues what might be the issue, or if there is any common logic to default scope that you think might cause errors and could quickly look at in the connector logic.

A few other tests are failing but I know exactly why, and they aren't related to the connector but instead to things like odbc connector error structure and encoding of database columns. I will probably have to make a few PRs to the juggler to do some checks for if the the connector name == ibmi like many connectors already do.

Looking forward to getting this out in the wild, any guidance you could give would be great!

bajtos · 2019-10-01T16:17:18Z

@markdirish I am often overwhelmed with GitHub notifications, sorry for responding so late. Feel free to ping me via Slack in the future to get a faster response.

I'll try to set aside some time later this week or next week to review the new codebase.

In the meantime, can you update your dependencies like async and most notably strong-globalize to their latest published versions? strong-globalize removed the heavy CLI parts to a standalone package in one of the recent semver-major versions. By upgrading your connector to use the latest version, you will significantly reduce the size of dependencies installed to projects using this connector.

markdirish · 2019-10-02T00:25:44Z

Totally understand, I feel the same way. Thanks for the Slack permission, I might exercise that from time to time!

I will update these dependencies tomorrow and add to the PR, I'm always a proponent of reducing the size of node_modules

bajtos · 2019-11-29T09:09:51Z

@markdirish sorry for keeping you waiting for so long. What's the current status of your work? Where can I find the latest codebase to review? (I remember there was some confusion around the relation between https://github.com/strongloop/loopback-connector-ibmi and https://github.com/markdirish/loopback-connector-ibmi)?

markdirish · 2019-12-10T13:54:17Z

PRs made to fix some tests for loopback-connector-ibmi:

loopbackio/loopback-datasource-juggler#1810
loopbackio/loopback-datasource-juggler#1811

markdirish · 2019-12-10T18:08:59Z

Issues I am still encountering:

The underlying connector for loopback-connector-ibmi is totally asynchronous. It is called with either odbc.connect or odbc.pool, which asynchronously return a connection or a pool.

However, in order to get a connection, many of the datasource-juggler tests run: db = getSchema(), which itself is implemented in loopback-connector-ibmi (modeled off of postgres and mysql connectors) as:

global.getDataSource = global.getSchema = function(options) {
  if (db === undefined) {
    db = new DataSource(require('..'), config);
  }
  return db;
};

The problem is, it checks if db is undefined (such as when the first test suite is run), and if it is, it will create a new DataSource (which internally should call connector.initialize). But the function (like all constructors) is synchronous, and doesn't wait for connector.initialize to return. I have been getting around this by calling my own tests first, and by the time my tests return there is at least one connection created in the connection pool, so tests don't fail. But if I try to run tests in isolation (especially with v4 of the juggler), it will call getSchema(), which then returns before the connector initialization has completed.

How are other connectors getting around this (possibly they use synchronous/constructor connections?), and how can I get around it?

The only tests that are actually failing outside of the above connector issues are the default-scope.test.js suite. For instance, the should apply default scope test fails. It appears the queries being fired off are creating the proper tables (Product, Widget, Category, Tool, Thing, Person). and then create Tool Z, Widget Z, Tool A, Widget A, and Widget B in the proper tables. But the line

Product.findById(ids.toolA, function(err, inst) { ...

produces the query:

SELECT "name","kind","description","active","id","categoryId","personId" FROM "Product" WHERE "id"=? ORDER BY "name";  FETCH FIRST 1 ROWS ONLY

(and binding 2 to the parameter marker). The problem is, the Tool it is looking for exists in the Tools table, but not in the Product table. So the query returns nothing (since the Product table is empty) and the test fails. I think there is some wider issue with relationships going on, but might need a little more expertise to know what.

dhmlau · 2019-12-10T18:26:14Z

@strongloop/sq-lb-apex @bajtos, any insights on the above comment? Thanks.

markdirish · 2019-12-10T18:28:38Z

The code in question can be found at https://github.com/markdirish/loopback-connector-ibmi

In regards to the first issue, it seems to run fine if I run a .only on v3 of the juggler. But if I do the same thing on the same test in v4, it doesn't wait for the pool to be initialized before trying to run tests, which causes a bunch of immediate failures and then a premature exit.

Is it a difference between the versions of the juggler, or perhaps how where the tests are? v3 datasource-juggler code lives in /deps/juggler-v3/node-modules, while the v4 tests are run directly from /node-modules (the main packages node-modules). I think this mirrors how other connectors run both versions of the datasource juggler.

markdirish · 2019-12-10T20:50:23Z

When I run just default scope tests for v3, with trace stacks when: 1. getSchema is called, 2. when ibmiconnector.initialize is called, and 3. when the call to odbc.pool returns inside ibmiconnector.initialize:

Trace: getSchema
    at global.getDataSource.global.getSchema (/home/mirish/loopback/loopback-connector-ibmi/test/init.js:23:11)
    at Context.<anonymous> (/home/mirish/loopback/loopback-connector-ibmi/deps/juggler-v3/node_modules/loopback-datasource-juggler/test/default-scope.test.js:58:10)
    at callFnAsync (/home/mirish/loopback/loopback-connector-ibmi/node_modules/mocha/lib/runnable.js:415:21)
    at Hook.Runnable.run (/home/mirish/loopback/loopback-connector-ibmi/node_modules/mocha/lib/runnable.js:357:7)
    at next (/home/mirish/loopback/loopback-connector-ibmi/node_modules/mocha/lib/runner.js:384:10)
    at Immediate._onImmediate (/home/mirish/loopback/loopback-connector-ibmi/node_modules/mocha/lib/runner.js:425:5)
    at runCallback (timers.js:705:18)
    at tryOnImmediate (timers.js:676:5)
    at processImmediate (timers.js:658:5)
Trace: intialize
    at Object.exports.initialize (/home/mirish/loopback/loopback-connector-ibmi/lib/ibmiconnector.js:1373:11)
    at DataSource.setup (/home/mirish/loopback/loopback-connector-ibmi/deps/juggler-v3/node_modules/loopback-datasource-juggler/lib/datasource.js:502:19)
    at new DataSource (/home/mirish/loopback/loopback-connector-ibmi/deps/juggler-v3/node_modules/loopback-datasource-juggler/lib/datasource.js:138:8)
    at global.getDataSource.global.getSchema (/home/mirish/loopback/loopback-connector-ibmi/test/init.js:25:10)
    at Context.<anonymous> (/home/mirish/loopback/loopback-connector-ibmi/deps/juggler-v3/node_modules/loopback-datasource-juggler/test/default-scope.test.js:58:10)
    at callFnAsync (/home/mirish/loopback/loopback-connector-ibmi/node_modules/mocha/lib/runnable.js:415:21)
    at Hook.Runnable.run (/home/mirish/loopback/loopback-connector-ibmi/node_modules/mocha/lib/runnable.js:357:7)
    at next (/home/mirish/loopback/loopback-connector-ibmi/node_modules/mocha/lib/runner.js:384:10)
    at Immediate._onImmediate (/home/mirish/loopback/loopback-connector-ibmi/node_modules/mocha/lib/runner.js:425:5)
    at runCallback (timers.js:705:18)
    at tryOnImmediate (timers.js:676:5)
    at processImmediate (timers.js:658:5)
Trace: pool created
    at odbc.pool (/home/mirish/loopback/loopback-connector-ibmi/lib/ibmiconnector.js:1379:13)
    at poolObj.init (/home/mirish/loopback/loopback-connector-ibmi/node_modules/odbc/lib/odbc.js:43:5)
    at Pool.init (/home/mirish/loopback/loopback-connector-ibmi/node_modules/odbc/lib/Pool.js:203:14)

And then, since the pool has been created (at least 1 open connection has been returned), it executes fine.

When I run the same code, with the same console.trace calls in v4, I get:

  juggler-v4
    default scope
Trace: getSchema
    at global.getDataSource.global.getSchema (/home/mirish/loopback/loopback-connector-ibmi/test/init.js:23:11)
    at Context.<anonymous> (/home/mirish/loopback/loopback-connector-ibmi/node_modules/loopback-datasource-juggler/test/default-scope.test.js:58:10)
    at callFnAsync (/home/mirish/loopback/loopback-connector-ibmi/node_modules/mocha/lib/runnable.js:415:21)
    at Hook.Runnable.run (/home/mirish/loopback/loopback-connector-ibmi/node_modules/mocha/lib/runnable.js:357:7)
    at next (/home/mirish/loopback/loopback-connector-ibmi/node_modules/mocha/lib/runner.js:384:10)
    at Immediate._onImmediate (/home/mirish/loopback/loopback-connector-ibmi/node_modules/mocha/lib/runner.js:425:5)
    at runCallback (timers.js:705:18)
    at tryOnImmediate (timers.js:676:5)
    at processImmediate (timers.js:658:5)
Trace: intialize
    at Object.exports.initialize (/home/mirish/loopback/loopback-connector-ibmi/lib/ibmiconnector.js:1373:11)
    at DataSource.setup (/home/mirish/loopback/loopback-connector-ibmi/node_modules/loopback-datasource-juggler/lib/datasource.js:517:19)
    at new DataSource (/home/mirish/loopback/loopback-connector-ibmi/node_modules/loopback-datasource-juggler/lib/datasource.js:146:8)
    at global.getDataSource.global.getSchema (/home/mirish/loopback/loopback-connector-ibmi/test/init.js:25:10)
    at Context.<anonymous> (/home/mirish/loopback/loopback-connector-ibmi/node_modules/loopback-datasource-juggler/test/default-scope.test.js:58:10)
    at callFnAsync (/home/mirish/loopback/loopback-connector-ibmi/node_modules/mocha/lib/runnable.js:415:21)
    at Hook.Runnable.run (/home/mirish/loopback/loopback-connector-ibmi/node_modules/mocha/lib/runnable.js:357:7)
    at next (/home/mirish/loopback/loopback-connector-ibmi/node_modules/mocha/lib/runner.js:384:10)
    at Immediate._onImmediate (/home/mirish/loopback/loopback-connector-ibmi/node_modules/mocha/lib/runner.js:425:5)
    at runCallback (timers.js:705:18)
    at tryOnImmediate (timers.js:676:5)
    at processImmediate (timers.js:658:5)

BUT it starts executing SQL statements here, not waiting until pool returns. So it starts erroring immediately.

Still digging...

markdirish · 2019-12-11T19:23:34Z

Issue 1, with connections trying to run SQL commands before the datasource has connected, are caused by the following line:

https://github.com/strongloop/loopback-datasource-juggler/blob/master/lib/datasource.js#L1080-L1086

This logic did not exist in v3. In v3, everything seems to execute in order, but in v4 it doesn't wait for the pool to connect, causing errors.

bajtos · 2019-12-12T14:00:23Z

The problem is, it checks if db is undefined (such as when the first test suite is run), and if it is, it will create a new DataSource (which internally should call connector.initialize). But the function (like all constructors) is synchronous, and doesn't wait for connector.initialize to return. I have been getting around this by calling my own tests first, and by the time my tests return there is at least one connection created in the connection pool, so tests don't fail. But if I try to run tests in isolation (especially with v4 of the juggler), it will call getSchema(), which then returns before the connector initialization has completed.

How are other connectors getting around this (possibly they use synchronous/constructor connections?), and how can I get around it?

Juggler supports asynchronous connector initialization, that's why many methods (including automigrate as you pointed out above) call this.ready to defer the actual execution until the connection is established.

In the constructor (the exported initialize function), you should synchronously create a connector instance in an unconnected state and optionally start the connecting process in background (depending on lazyConnect flag).

The connector should implement connect method, which is an asynchronous method used to establish the connection.

Here are the relevant bits from the PostgreSQL connector:

See also the code in DataSource class which is dealing with different connector states (connecting, connected, etc.):

https://github.com/strongloop/loopback-datasource-juggler/blob/d19e189fd75d1270a67b82267682ebe96bdd8a20/lib/datasource.js#L307-L363

ssue 1, with connections trying to run SQL commands before the datasource has connected, are caused by the following line:

https://github.com/strongloop/loopback-datasource-juggler/blob/master/lib/datasource.js#L1080-L1086

This logic did not exist in v3. In v3, everything seems to execute in order, but in v4 it doesn't wait for the pool to connect, causing errors.

IIRC, in v3, it was the responsibility of the caller of automigrate to ensure the connection was already established. In v4, we removed this requirement and improved automigrate to automatically connect first (if needed).

I believe this works well for existing connectors. Now since it does not work for ibmi, then I suspect the bug will be in this connector, but it's also possible the implementation in juggler is flawed 🤷‍♂

bajtos · 2019-12-12T14:53:01Z

How are other connectors getting around this (possibly they use synchronous/constructor connections?), and how can I get around it?

See #9 (comment)

bajtos · 2020-01-06T10:02:03Z

I am closing this issue as done via #9.

ThePrez · 2020-01-06T17:19:58Z

@markdirish , are you planning to open a PR to https://github.com/strongloop/loopback.io?

markdirish · 2020-01-06T17:55:43Z

I guess I didn't realize the site was on the GitHub! I will work on a PR to post about the new connector

markdirish mentioned this issue Sep 20, 2019

Version 1.0.0 of loopback-connector-ibmi #9

Merged

2 tasks

bajtos assigned bajtos and markdirish Nov 29, 2019

bajtos added the help wanted Extra attention is needed label Nov 29, 2019

bajtos closed this as completed Jan 6, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Review feedback #7

Review feedback #7

bajtos commented Aug 20, 2019

markdirish commented Aug 20, 2019

dhmlau commented Aug 20, 2019

markdirish commented Aug 20, 2019

dhmlau commented Aug 20, 2019

kadler commented Aug 20, 2019

markdirish commented Sep 6, 2019

markdirish commented Sep 6, 2019

markdirish commented Sep 6, 2019

markdirish commented Sep 13, 2019

bajtos commented Oct 1, 2019

markdirish commented Oct 2, 2019

bajtos commented Nov 29, 2019

markdirish commented Dec 10, 2019

markdirish commented Dec 10, 2019

dhmlau commented Dec 10, 2019

markdirish commented Dec 10, 2019

markdirish commented Dec 10, 2019

markdirish commented Dec 11, 2019

bajtos commented Dec 12, 2019

bajtos commented Dec 12, 2019

bajtos commented Jan 6, 2020

ThePrez commented Jan 6, 2020

markdirish commented Jan 6, 2020

Review feedback #7

Review feedback #7

Comments

bajtos commented Aug 20, 2019

markdirish commented Aug 20, 2019

dhmlau commented Aug 20, 2019

markdirish commented Aug 20, 2019

dhmlau commented Aug 20, 2019

kadler commented Aug 20, 2019

markdirish commented Sep 6, 2019

markdirish commented Sep 6, 2019

markdirish commented Sep 6, 2019

markdirish commented Sep 13, 2019

bajtos commented Oct 1, 2019

markdirish commented Oct 2, 2019

bajtos commented Nov 29, 2019

markdirish commented Dec 10, 2019

markdirish commented Dec 10, 2019

dhmlau commented Dec 10, 2019

markdirish commented Dec 10, 2019

markdirish commented Dec 10, 2019

markdirish commented Dec 11, 2019

bajtos commented Dec 12, 2019

bajtos commented Dec 12, 2019

bajtos commented Jan 6, 2020

ThePrez commented Jan 6, 2020

markdirish commented Jan 6, 2020