Skip to content
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

Implement metadata handling #4

Merged
merged 5 commits into from
Nov 6, 2020
Merged

Conversation

mjbrisebois
Copy link
Contributor

Changes

@mjbrisebois mjbrisebois marked this pull request as ready for review November 3, 2020 23:34
Copy link
Contributor

@JettTech JettTech left a comment

Choose a reason for hiding this comment

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

Just a couple of non-blocking questions and clean-up recommendations. Overall, LTGM.

tests/unit/test_package.js Outdated Show resolved Hide resolved
src/index.js Outdated Show resolved Hide resolved
// {
// type: 'success',
// payload: 'I am a werewolf',
// metadata: {
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we expecting the response_id to be included in the metadata as a standard now? If so, can we add an example of the response_id being included here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, the data-translator is a general-use translator, the response_id is implementation-specific between Chaperone and Envoy.

Comment on lines +156 to +161
if ( arguments.length === 2 ) {
if ( value === undefined ) {
const previous_value = this._metadata[key];
delete this._metadata[key];
return previous_value;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you put this into context for me? When might the metadata return without a value, and why how do we know it's response_id will be/should be the exact same?

Copy link
Contributor Author

@mjbrisebois mjbrisebois Nov 6, 2020

Choose a reason for hiding this comment

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

I don't fully understand the question. The metadata value is always returned, but the value may have been set to undefined (via delete)

mjbrisebois and others added 3 commits November 6, 2020 07:41
Co-authored-by: Lisa Jetton <lisa.jetton@holo.host>
Co-authored-by: Lisa Jetton <lisa.jetton@holo.host>
@mjbrisebois mjbrisebois merged commit b4ff365 into develop Nov 6, 2020
@mjbrisebois mjbrisebois deleted the metadata-handling-2020-11-03 branch November 6, 2020 14:49
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.

Metadata parsing is broken
2 participants