Skip to content

Reworked a bit the Extension interface #158

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Jun 1, 2020

Conversation

slinkydeveloper
Copy link
Member

Look at #122

This change:

  • Change the signature of readFromEvent to accept CloudEventExtensions
  • Remove asMap() and let Extension extend CloudEventExtensions. Because the number of attributes for a materialized extension is always the same, allocating a map to provide an unstructrured view (needed to write the materialized extension to the cloud event builder) is useless. This solution makes more efficient the implementation of such materialized extensions

Signed-off-by: Francesco Guardiani [email protected]

Signed-off-by: Francesco Guardiani <[email protected]>
@slinkydeveloper
Copy link
Member Author

@bsideup mind giving a look?

/**
* Materialized CloudEvent Extension interface to read/write the extension attributes key/values.
*/
public interface Extension extends CloudEventExtensions {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This sounds wrong. Extension extends CloudEventExtensions smells.

Especially given that it will look like:

Extension ext = ...;

ext.getExtension("OMG WTF!");
ext.getExtensionNames(); // whaaaat?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how about:

public interface Extension {

    void readFrom(CloudEventExtensions extensions);

   Object getValue(String key);

   Set<String> getKeys();

}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Much better :)

WDYT about Iterable<String> getKeys();? Since extension may define a dynamic set of keys, Set here can be a bit restrictive, and it does not look like we need to have Set's characteristics in this case

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To me it doesn't sound explicative Iterable<String>, because Iterable can contain duplicates. Because we're returning a key set, to me it makes sense using the Set<String> type, exactly like i expect a Map to return the keys as a set

Copy link
Member Author

@slinkydeveloper slinkydeveloper May 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the other hand, I don't think it would be a bad idea to let this interface extend Iterable<Map.Entry<String, Object>> or have a method like iter() that returns such Iterable

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And btw such Iterable can be default implemented

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Iterable<Map.Entry<String, Object>> would require allocations.

Okay, let's keep Set then.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Iterable<Map.Entry<String, Object>> would require allocations.

Sure, it's like "use it at your own risk" 😄

…terialized extension

Signed-off-by: Francesco Guardiani <[email protected]>
Signed-off-by: Francesco Guardiani <[email protected]>
@slinkydeveloper slinkydeveloper merged commit 73d90d1 into cloudevents:master Jun 1, 2020
@slinkydeveloper slinkydeveloper deleted the issues/122 branch June 1, 2020 07:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants