search

guardian / scribe-plugin-noting

9 stars 3 forks source link

Explicit type checking hides information #52

Closed robinedman closed 9 years ago

robinedman commented 9 years ago

At the moment we have type checking like:

if (!isVFocus(focus)) {
    throw new Error('Only a valid VFocus element can be passed to findEntirenote');
}

(https://github.com/guardian/scribe-plugin-noting/blob/jp-refactor-note-toggle/src/utils/noting/find-entire-note.js#L23)

When this is thrown we don't get any info about what focus actually is. In this sense it's harder to debug what actually happened than if we wouldn't have had it all.

Could we either:

  1. Standardise on a type check function that will display relevant details like parameters in the error message?
  2. Or simply rely on our unit tests instead of these checks?
robinedman commented 9 years ago

I tend to gravitate towards less ceremony and more duck typing myself (i.e. the 2nd option) but I'm fine with either! :)

hmgibson23 commented 9 years ago

Was just about to mention this. Would be nice to be able to debug these with some more information. Having said that, we are mostly just ignoring Sentry errors so I'm not sure this will help that much.

cutandpastey commented 9 years ago

Is fixed in https://github.com/guardian/scribe-plugin-noting/pull/60, I'll close this when it's merged.

rrees commented 9 years ago

@robinedman I think I would lean towards option two as well and if you still have a type or validity check push it to boundaries of the code path

cutandpastey commented 9 years ago

Right now we have a handleError function which gives us a full print out a focus which causes error. I think @theefer has a very good point here: https://github.com/guardian/scribe-plugin-noting/pull/60#discussion-diff-22654506 I'll migrate to this solution at a later date