Refactor contribution serializer and validation #94

2019-04-11T11:06:42Z

raucao commented

2019-04-11 11:06:42 +00:00

(Migrated from github.com)

And fix seeds.

raucao commented

2019-04-11 11:08:23 +00:00

(Migrated from github.com)

I set this to WIP, because I'm going to add proposal validation and valid proposal seeds to this as well.

raucao commented

2019-04-11 11:51:35 +00:00

(Migrated from github.com)

Ready for review!

fsmanuel (Migrated from github.com) reviewed 2019-04-11 11:58:35 +00:00

lib/serializers/contribution.js

						
				@@ -72,1 +51,4 @@

				  }

				  /**

				   * Validate serialized data against schema

fsmanuel (Migrated from github.com) commented

2019-04-11 11:58:35 +00:00

What about:

  serialize (stringify=true) {
    // the current implementation

    return stringify ? JSON.stringify(data, null, 2) : data;
  }

  validate () {
    const serialized = this.serialize(false);
    const valid = validator.validate(serialized, schemas['contribution']);
    return valid ? Promise.resolve() : Promise.reject(validator.error);
  }

What about: ```js serialize (stringify=true) { // the current implementation return stringify ? JSON.stringify(data, null, 2) : data; } validate () { const serialized = this.serialize(false); const valid = validator.validate(serialized, schemas['contribution']); return valid ? Promise.resolve() : Promise.reject(validator.error); } ```

bumi (Migrated from github.com) approved these changes 2019-04-11 21:34:35 +00:00

bumi (Migrated from github.com) left a comment

:shipit:
@fsmanuel what's with your comment, want to merge it?

:shipit: @fsmanuel what's with your comment, want to merge it?

raucao (Migrated from github.com) reviewed 2019-04-12 07:07:34 +00:00

lib/serializers/contribution.js

						
				@@ -72,1 +51,4 @@

				  }

				  /**

				   * Validate serialized data against schema

raucao (Migrated from github.com) commented

2019-04-12 07:07:34 +00:00

Let's try ro create smaller PRs for suggestions like this, instead of having them hold up larger branches from being merged.

(E.g. personally, I don't like unnamed options, so I'd probably propose to use an options object for this. But that's a completely different discussion than if the schema validation here is implemented good enough in general or not.)

What do you think?

Let's try ro create smaller PRs for suggestions like this, instead of having them hold up larger branches from being merged. (E.g. personally, I don't like unnamed options, so I'd probably propose to use an options object for this. But that's a completely different discussion than if the schema validation here is implemented good enough in general or not.) What do you think?

fsmanuel (Migrated from github.com) reviewed 2019-04-12 11:27:10 +00:00

lib/serializers/contribution.js

						
				@@ -72,1 +51,4 @@

				  }

				  /**

				   * Validate serialized data against schema

fsmanuel (Migrated from github.com) commented

2019-04-12 11:27:10 +00:00

Well I think that the initial idea to move the validation into the serializer was to get rid of the JSON.stringify ->JSON.parse thing for the validation. My suggestion is focusing on how we can prevent this.
Another thing I don't understand is why you removed the static function and don't use the class at all.

Well I think that the initial idea to move the validation into the serializer was to get rid of the `JSON.stringify` ->`JSON.parse` thing for the validation. My suggestion is focusing on how we can prevent this. Another thing I don't understand is why you removed the static function and don't use the class at all.

raucao (Migrated from github.com) reviewed 2019-04-12 11:32:57 +00:00

lib/serializers/contribution.js

						
				@@ -72,1 +51,4 @@

				  }

				  /**

				   * Validate serialized data against schema

raucao (Migrated from github.com) commented

2019-04-12 11:32:57 +00:00

Well I think that the initial idea to move the validation into the serializer was to get rid of the JSON.stringify ->JSON.parse thing for the validation. My suggestion is focusing on how we can prevent this.

That's not at all how I understood the idea. I merely refactored everything to be both shared modules and also do the validation in the class instead of outside.

Another thing I don't understand is why you removed the static function and don't use the class at all.

Not sure I understand. It's using only the class now, because I added more functions to it. I turned the static function into a prototype function, because that's actually using the class with prototype properties then.

> Well I think that the initial idea to move the validation into the serializer was to get rid of the `JSON.stringify` ->`JSON.parse` thing for the validation. My suggestion is focusing on how we can prevent this. That's not at all how I understood the idea. I merely refactored everything to be both shared modules and also do the validation in the class instead of outside. > Another thing I don't understand is why you removed the static function and don't use the class at all. Not sure I understand. It's using only the class now, because I added more functions to it. I turned the static function into a prototype function, because that's actually using the class with prototype properties then.

raucao (Migrated from github.com) reviewed 2019-04-12 11:33:53 +00:00

lib/serializers/contribution.js

						
				@@ -72,1 +51,4 @@

				  }

				  /**

				   * Validate serialized data against schema

raucao (Migrated from github.com) commented

2019-04-12 11:33:53 +00:00

By the way, in a way I also like validating exactly what's being stored in IPFS. I don't see it as a problem, and I don't see why a refactoring of that should keep us from merging now, as explained. I'm open to good reasons why the code in this PR doesn't work or is so bad it needs to be changed. But frankly, it seems like wasting time to me. This refactoring wouldn't even affect the code using the class from the outside. Let alone clients using the wrapper.

By the way, in a way I also like validating *exactly* what's being stored in IPFS. I don't see it as a problem, and I don't see why a refactoring of that should keep us from merging now, as explained. I'm open to good reasons why the code in this PR doesn't work or is so bad it needs to be changed. But frankly, it seems like wasting time to me. This refactoring wouldn't even affect the code using the class from the outside. Let alone clients using the wrapper.

fsmanuel (Migrated from github.com) reviewed 2019-04-12 12:33:24 +00:00

lib/serializers/contribution.js

						
				@@ -72,1 +51,4 @@

				  }

				  /**

				   * Validate serialized data against schema

fsmanuel (Migrated from github.com) commented

2019-04-12 12:33:24 +00:00

I was referring to this https://github.com/67P/kredits-contracts/pull/92#discussion_r274168386
So my suggestion adds little to archive this. I modified to code above to better illustrate the idea. If you disagree merge it as is.

Let's try to create smaller PRs for suggestions like this, instead of having them hold up larger branches from being merged.

I think reviews are the place to raise this discussions, aren't they?

I was referring to this https://github.com/67P/kredits-contracts/pull/92#discussion_r274168386 So my suggestion adds little to archive this. I modified to code above to better illustrate the idea. If you disagree merge it as is. > Let's try to create smaller PRs for suggestions like this, instead of having them hold up larger branches from being merged. I think reviews are the place to raise this discussions, aren't they?

fsmanuel (Migrated from github.com) reviewed 2019-04-12 12:35:00 +00:00

lib/serializers/contribution.js

						
				@@ -72,1 +51,4 @@

				  }

				  /**

				   * Validate serialized data against schema

fsmanuel (Migrated from github.com) commented

2019-04-12 12:35:00 +00:00

That being said. I like your implementation!

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: kredits/contracts#94