Skip to content

Replace Map<String,String[]> with serializable Metadata#36

Closed
jnioche wants to merge 5 commits intomasterfrom
metadata
Closed

Replace Map<String,String[]> with serializable Metadata#36
jnioche wants to merge 5 commits intomasterfrom
metadata

Conversation

@jnioche
Copy link
Copy Markdown
Contributor

@jnioche jnioche commented Dec 11, 2014

Having a wrapper for Metadata would be a bit more elegant than moving the Map<String,String[]> around.
This initial implementation adds Metadata and removes the KeyValues utility. Later on we can replace all occurrences of Map<String,String[]> and systematically use Metadata instead.

@jnioche
Copy link
Copy Markdown
Contributor Author

jnioche commented Dec 12, 2014

@GuiForget any thoughts on this?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I would check for null and make a copy of the map

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Ok given that for now you use the Metadata class just as a wrapper, making a copy isn't possible. Checking for null is probably a good idea still

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

this is temporary indeed. Am now throwing an exception when the value is null.

@jnioche
Copy link
Copy Markdown
Contributor Author

jnioche commented Dec 15, 2014

thanks for your comments Gui

@jnioche jnioche added this to the 0.4 milestone Dec 18, 2014
@jnioche jnioche modified the milestones: 0.5, 0.4 Dec 19, 2014
@jnioche
Copy link
Copy Markdown
Contributor Author

jnioche commented Dec 19, 2014

Quite a large change, I think we should release 0.4 first then merge this.
@GuiForget @DigitalPebble/committers-crawler

@jakekdodd
Copy link
Copy Markdown
Contributor

Now that we've released 0.4, should we make this priority 1? I think several other issues are affected by the new metadata scheme

@GuiForget
Copy link
Copy Markdown

Yes yes yes!!! 💃

My only concern is should we just go further and replace the use of Map<String, String[]> with Metadata class right away?

@jnioche
Copy link
Copy Markdown
Contributor Author

jnioche commented Jan 29, 2015

+1 to make it top priority. I will take care of this.

replace the use of Map with Metadata class right away

that's what the old PR was attempting to do, might as well do it all in one go.

Can't output it from a bolt and not get the following bolt to handle them, can't we?

@jnioche
Copy link
Copy Markdown
Contributor Author

jnioche commented Jan 29, 2015

Opened a new PR #78

@jnioche jnioche closed this Jan 29, 2015
@jnioche jnioche deleted the metadata branch February 4, 2015 14:55
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