Skip to content

Conversation

@agentgonzo
Copy link
Member

More of a suggestion/discussion than a real PR.

@justinabrahms , let me know your thoughts.

@agentgonzo agentgonzo requested a review from justinabrahms July 1, 2022 12:11
.collect(Collectors.toMap(Map.Entry::getKey, e -> (Boolean)e.getValue()))
);
// Hmmm, this won't work as we already have a string type.
// I would recommend changing the type to Map<String, JsonNode> from Jackson
Copy link
Member

Choose a reason for hiding this comment

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

I'm a jackson newbie, but my understanding is that jsonnode's are an immutable json tree structure. That seems fine to me for this.

@beeme1mr
Copy link
Member

@agentgonzo, this seems like a nice change. Would you be able to get it into a mergeable state?

@justinabrahms any concerns about this?

@justinabrahms
Copy link
Member

@beeme1mr No strong concerns. There are backwards incompatibilities here, which I don't love, but it's still early days so.. seems inevitable?

@justinabrahms
Copy link
Member

@agentgonzo Do you have an intent to finish up this PR? Or should I close it?

@beeme1mr
Copy link
Member

beeme1mr commented Aug 1, 2022

Closing due to inactivity.

@beeme1mr beeme1mr closed this Aug 1, 2022
@agentgonzo
Copy link
Member Author

This completely fell off my radar. I'm away for a week, but will try to get a decent mergeable PR ready when I get back

@beeme1mr
Copy link
Member

beeme1mr commented Aug 3, 2022

@agentgonzo sounds good! Feel free to reopen the PR when it's ready.

justinabrahms added a commit that referenced this pull request Aug 8, 2022
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.

3 participants