chore: formatter.js es6

[grant]

Test PR to ensure tests work.

[helio-frota]

Hi,

I have a question on this. Why the formatter is required ?

One of the functions is doing nothing, only returning the parameter.
The other function is calling JSON.stringify.

The tests keep passing when changing from:

Cloudevent.prototype.format = function() {
  // Check the constraints
  this.spec.check();

  // To run asData()
  this.getData();

  // Then, format
  return this.formatter.format(this.spec.payload);
};

to

Cloudevent.prototype.format = function() {
  // Check the constraints
  this.spec.check();

  // To run asData()
  this.getData();

  // Then, format
  return this.spec.payload;
};

And the toString function is basically calling the other toString function:

Cloudevent.prototype.toString = function() {
  return this.formatter.toString(this.spec.payload);
};

So why not to remove formatter directly ?

Is that something required to have due specification or something ?

[grant]

Hi, I believe there are lots of files in this repo that aren't required. This PR simply updates the file to ES6.

Perhaps we should create a bigger PR to remove unused pieces of code or simplifying files.

[helio-frota]

Perhaps we should create a bigger PR to remove unused pieces of code or simplifying files.

Or small PRs to remove concise parts to be later easily traceable via git log 👍

[lance]

@helio-frota good question. If I can speculate, I imagine this is a pattern so that the CloudEvent object doesn't need to know what formatters a given spec provides. This way data can be provided in multiple formats.

Your question exposes some things to think about.

1   Cloudevent.prototype.format = function() {
2      // Check the constraints
3      this.spec.check();
4
5      // To run asData()
6      this.getData();
7
8      // Then, format
9      return this.spec.payload;
10  };

On line 6, the call to this.getData() is either useless or expects some kind of side effect - a code smell. Then, proxying calls to spec.check() and spec.payload seems like maybe all of this should be encapsulated in spec itself and this function should simply be proxying to this.spec.format() which can use an explicit formatter or not, and do all of the validation checks, etc. instead of having that knowledge in Cloudevent.

It seems that the only format in use is JSON and there's no apparent way to ask for the data in any other format. So given that, maybe the format() function should take a format type, defaulting to JSON.

The Cloudevent object exposes a formats property. Maybe this was the thinking when the code was originally developed.

[lance]

LGTM - I will open an issue to continue the discussion around the overall code structure.

[helio-frota]

yeah, I've added a thumbs up on your comment to simplify the communication 😄
But good catch on the code smell!

[lance]

@grant minor nit - but I wouldn't call this a feat in the commit log. Too late now, I realize, but I suggest chore instead. With the feat prefix, this commit will show up in the changelog as a new feature - and bump the minor version number as a result. Probably not a big deal for now, since the minor version will likely bump anyway with the next release. But something to keep in mind.

[grant]

@grant minor nit - but I wouldn't call this a feat in the commit log. Too late now, I realize, but I suggest chore instead. With the feat prefix, this commit will show up in the changelog as a new feature - and bump the minor version number as a result. Probably not a big deal for now, since the minor version will likely bump anyway with the next release. But something to keep in mind.

Ah, got it. Thanks for the FYI.