Redis records

[mattgstevens]

addresses #7

adds two concrete record types Content and Invoice, and uses them

[mattgstevens]

Moving out to PR comments to address the "validation framework" conversation started inline.

The validateRecord idea started in records/Invoice.js file, but I will immediately be using it in records/Content.js to do the same kind of work to provide the Api route to register content.

If there was no validateRecord, there would be duplicated code (aka i would be very tempted to copy paste it into other record definitions) since you will loop over all properties, call them one by one, and possibly react to validation errors.

With validateRecord we have an interface for all Records to implement: there should be a properties object where all keys are propNames and the value is a function to validate it.

If you still feel that this is too heavy then I will remove validateRecord.js and write it for both Invoice and Content in their definition

[fromheten]

Ok - but functions compose already. We can have an isValidRecord predicate. That can be reused, and called from any predicate verifying records. That's basically what we are doing, but implicit with this type system. So isValidInvoice would call isValidRecord if an invoice needs to be a valid record.

I don't know man, do you think these ideas make sense? Would it make the code simpler or more complex?

[fromheten]

A function can be reused in any way - the predicates could be ripped out and used elsewhere. If we have a framework that objects need to conform to, we depend on having that structure everywhere.

[mattgstevens]

Without a code example I'm not 100% following you. There is some magic happening with Record.properties object being required which could be removed.

Would this make more sense?

// file: records/Invoice.js
const schema = {
  address: Predicates.isBitcoinAddress,
  ...
}
Invoice = {
  find: () => ...
  save: (data) => {
    Invoice.validate(data);
    //redisDb.saveThisThing(data) ...
  }
  validate: require('util/validate').bind(null, schema)
}

module.exports = Invoice;

Instead of having isValidInvoice in the utils/Predicates.js I would prefer to have a record file for data we are saving / reading as this is convenient place for validation and I/O and easier to read in my opinion (both as a source file and at import time)

[fromheten]

I think what I'm wondering is why do the validate: require('util/validate').bind(null, schema) rather than validate: (invoice) => isValidRecord(invoice) && isCoolAndSmellsNice(invoice) && isOrganicallyProducedAndLocallyFarmed(invoice) etc. If you ask me, looking at the latter it's a no brainier what is required for a valid invoice.

But again, I'm not sure I'm more right or anything so please only do if you agree! Don't want to be mr pain in the ass code reviewer, just see if things can be simpler and less coupled. The same things would be happening in both cases, but your solution has the list of preficates in the Invoice class definition, called by the predicate framework. So yeah if you think this solution is the simpler way I say LGTM if it's tested. I'm writing on mobile now so can't really make a big code example. What I propose is in the same fashion as _.isString, but for our data structures (which are collections of primitives).

Idk worrying if I'm too anal about this, let me know bud.

[fromheten]

Because functions compose (by just calling another function as their body) they are super flexible and don't depend on a framework or superclasses. That's just a nice feature, and since we run JS on clients we can just use the same function there - even without it having to inherent the knowledge about what "record" means and so.

[fromheten]

But yeah don't know if I'm being a PITA about this - if you think this is the best way I'm all ok with that 👍

[fromheten]

Ok man, first of all I'm sorry about taking forever. On my first read thru I didn't understand what was happening, but after having read thru a few times I see it.

I made a diff with what I meant when we were talking earlier, it's in #19. Honestly, I leave it up to you to decide what version you like the best. What tripped me up here I realize is that I thought the functions in Types.js would return bools, and I didn't read them thru properly enough to realize that they returned the message describing what was missing, if there was something missing.

So - let me know which version you like most. I made the #19 be a pull request to merge into this branch - and then we'd merge this one into master to have all be tip top, and peace return to the galaxy.

[fromheten]

Code that does not run

So I commited 868334e6abc390faaf160e85afe9331b66db21f8. The code was not running because there were a few syntax errors (like missing , in objects and require( that didn't contain the .. in the beginning of their path, therefore not resolving). The linter was also unsatisfied. I assume the code didn't run on your machine either?

I propose we add lint to the test task - that will make the CI run the linter as well right? Test task would then be "test": "npm run lint && NODE_PATH=. tape src/**/*.spec.js",

Validation framework

I'm still not sure we are solving more problems than we are adding complexity with the 'validation framework'. Are you sure it's better than just having functions that return boolean and before they return false they also write to stderr?

function isPancakeRecipe (recipe) {
   // console.error returns undefined, which is falsy
  ('flour' in recipe) ? return true : !!console.error('Recipe does not contain flour', recipe)
}

I could very well be wrong (from past experience in life, I often am). But it seems like an optimization for some case we don't have yet. Keep It Stupid Simple and all that tango :).

Even more I think the predicates are a yes/no question we ask the machine. We can then act on it's answer. We can assert(isPancakeRecipe(recipe), "Err: Won't save non-pancake recipe to DB" + recipe), and thus make sure the yes/no question does only one thing - answer the question we ask it.

On this issue I feel like we have discussed enough - so if you think it is needed then let's merge that.

[fromheten]

What do you think of the idea of having a function that returns a list of all errors in a record?

function errorsInInvoice (invoice) {
  var errors = []
  if (!predicates.isBitcoinAddress(invoice.address)) errors.push(invoice.address + " is not a valid bitcoin address")
  if (!_.isString(invoice.contentId)) errors.push(invoice.contentId + " must be a string")
  if (!predicates.isBitcoinPrivateKey(invoice.privateKey)) errors.push(invoice.privateKey + " is not a valid WIF bitcoin private key")

  return errors
}
function isValidInvoice (invoice) {return errorsInInvoice(invoice) === 0}

No framework needed, and we can show users a list of what errors they make. No need to specify a properties object for every record type, and format it in a way that the framework can consume.

[fromheten]

console.assert(isValidInvoice(invoice), errorsInInvoice(invoice)) to get some nice error messages when it's invalid. Pure λ

[fromheten]

requirements = {
  address: {
    predicate: predicates.isBitcoinAddress,
    errorMsg: "address needs to be a valid bitcoin address"
  }
}

Then have a function that maps thru requirements.keys.

Really whatever about all of these, let's just KISS and get on with it :). Sorry for being a pita about it but happy you enjoy coding with me bud :v:

[mattgstevens]

So came back to this today, and Im going forward with the isValid mechanism for choosing between "hard validate" (throwing errors) and "soft validate" (predicate style boolean returns). This also speaks to the last comments from you about "a function that returns a list of all errors in a record" and "requirements map object".

There are some bonus commits at the end:

• why our requires were still relative d930f2922651492467a099808a022680d8e0ddc1
• a npm script for repl (with NODE_PATH) 5cfdd7d1bc3e4137986cf9e45b2d879d67c849fc
• npm script test will run lint and testsuite
• better naming for the isValid flag => from boolean to options object "{ throwsError: boolean }"