Adding asMap to ExtensionMetadata Interface #288
Conversation
Merge from Master
| Map<String, String[]> map = new HashMap<>(); | ||
|
|
||
| if (loc != null) { | ||
| map.put("loc", new String[]{ loc.toString() }); |
There was a problem hiding this comment.
For this, and all other xxxAttributes classes, are there well defined map key name constants we could use?
If not, then I would be in favor of adding them as public constants to the xxxAttribute classes, to avoid raw string constant usage in consumer code.
There was a problem hiding this comment.
There are not constants that I could fine. I'll add some and use those instead of strings.
| if (params != null) { | ||
|
|
||
| for (Entry<String, String> entry : params.entrySet()) { | ||
| map.put(entry.getKey(), new String[] { entry.getValue() }); |
There was a problem hiding this comment.
Are we guaranteed that non of these entries has a key = href? I think it would be safer and clearer to make the map key something like params.xxx
There was a problem hiding this comment.
Good call out, it is not a guarantee. I'll prefix these parameters to avoid conflicts and potential overwrites.
fixing a small issue in NewsAttributes.toString
|
@kkrugler This should be ready for another review. I addressed both points of feedback. Thanks again. |
| @@ -32,6 +34,10 @@ | |||
| * you to check for mistakes.</blockquote> | |||
| */ | |||
| public class LinkAttributes extends ExtensionMetadata { | |||
|
|
|||
| public static final String HREF = "href"; | |||
| public static final String PARAMS = "params.%s"; | |||
There was a problem hiding this comment.
My only comment here is that PARAMS is a bit different from the other keys. Not a big deal, but I'd probably call it PARAMS_PREFIX, and make its value just params.
In any case I'd add a comment explaining how it's used for the map key.
There was a problem hiding this comment.
Thinking about PARAMS_PREFIX a little more, does it make sense to decrease the visiblity? I'm not sure how useful it is outside of the class.
adding a comment explaining it's usage
|
am getting an error when running the tests _[ERROR] Failures: a quick debug shows that the equality of the results being based on identity (==) not equals to. @evanhalley could you please add a line to CHANGES.txt as part of the PR? Thanks! |
|
I know what happened with that test failure, I made the following change: I'll fix the unit tests. |
|
Building on my last response, I reverted my change to
|
|
hi @evanhalley, is more work needed on this or is it ready to be merged? It now passes the tests, thanks |
|
@jnioche this is ready to be merged. |
|
thanks again @evanhalley, your contribution is greatly appreciated |
Adding asMap to ExtensionMetadata interface. Implemented asMap in all subclasses.
Fixes #287