-
Notifications
You must be signed in to change notification settings - Fork 46
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #126 +/- ##
===========================================
+ Coverage 96.29% 96.57% +0.27%
===========================================
Files 2 2
Lines 432 438 +6
Branches 132 138 +6
===========================================
+ Hits 416 423 +7
+ Misses 16 15 -1
Continue to review full report at Codecov.
|
Also snuck in an update of webpack to fix some breaking changes and designated support for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, just a few thoughts on some of the style changes. Also we should switch the commands to all use npm rather than yarn, even if it doesn't really make a difference in package.json.
const net = nets[key] | ||
if (typeof net === 'object') { | ||
['registry', 'rpcUrl'].forEach((key) => { | ||
if (!net.hasOwnProperty(key)) throw new Error(`Malformed network config object, object must have '${key}' key specified.`) | ||
;['registry', 'rpcUrl'].forEach(key => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was this semicolon added by prettier? Shouldn't be necessary as it's the start of a block
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but not sure why. I'll look for the config option for that.
return this.signJWT({unsignedClaim, sub, riss, aud, vc, callback: callbackUrl, type: Types.VERIFICATION_SIGNATURE_REQUEST}) | ||
createVerificationSignatureRequest(unsignedClaim, { aud, sub, riss, callbackUrl, vc } = {}) { | ||
return this.signJWT({ | ||
unsignedClaim, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this line separation also done by prettier? I'm all for new lines but the really short ones look a little silly. The bikeshed should also be purple.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, done by prettier. I increased the preferred line width from 80 to 120 to get rid of other similar line splits. This one would be 129 wide on a single line and having 7 args in a row is a bit hard to visually parse at a glance. It kind of defeats the purpose of running something like prettier if we manually override individual lines, so maybe it would be most helpful to reaching a standard style by discussing this in terms of preferred config options?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah definitely agree, 120 line max seems reasonable to me!
And yeah I think automating the decisions is good, just was curious generally about if prettier makes all of those formatting changes itself
Added
https-did-resolver
dependency. Code changes are mostly formatting, except for registering the resolver on line 123 ofsrc/Credentials.js
. Couldn't figure out a reasonable way to test support for the new resolver in the unit tests of this repo so I created an example project to do a small integration test: https://github.com/uport-project/https-did-example.