-
Notifications
You must be signed in to change notification settings - Fork 16
change affiliation field from mandatory to optional #3
Conversation
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.
Please look at comments.
@@ -171,7 +171,9 @@ String getName() { | |||
} | |||
|
|||
String getMspid() { |
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.
I would be careful here and in other places as I don't think these are optional. If they're not present probably should throw some type of exeception.
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.
If the field is required, should I remove the checking?
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.
That's one option. I would think better throwing an exception with an error that the field was missing from the NetworkConfig document.
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.
Have you done some testing with this change ?
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.
I only do the testing with "affiliation" change from the Jason file. I will leave those fields untouched then
@@ -292,15 +299,21 @@ public String getEventURL() { | |||
} | |||
|
|||
public String getEnrollId() { |
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.
Again I think there would always be an id here
return value.getString("enrollId"); | ||
if(value.containsKey("enrollId")) | ||
return value.getString("enrollId"); | ||
return null; | ||
} | ||
|
||
public String getAffiliation() { |
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.
Here this seems appropriate
README.md
Outdated
|
||
## Hyperledger Fabric Java SDK Version | ||
This code is dependent on a 1.1.0-SNAPSHOT | ||
Requires Java SDK 1.1.0-SNAPSHOT |
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.
Probably should be Hyperledger Fabric Java SDK 1.1.0-SNAPSHOT
#2