Solve #31 Provide method to get all values for a key from all appende…

[westonal]

#31 get(key) Across Multiple Manifests

[dmarkov]

@westonized Thanks for the pull request, let me find a reviewer..

[dmarkov]

@pinaf please review, thanks

[pinaf]

@westonized could you please add a link to the original issue in the first PR comment?

[pinaf]

@westonized let's rename this to allValues

[westonal]

@pinaf OK

[pinaf]

@westonized thanks. 18 commments above.

[pinaf]

@westonized returnsImmutableListOfAllValues

[westonal]

@pinaf Well it's not immutable, it's just your own copy, an independent reference to anything held inside Manifest. I will go for returnsIndependentListOfAllValues. It would only be immutable if I combined a copy with a readonly, i.e. return Collections.unmodifiableList(new ArrayList<String>(this.multimap.get(key)))

[pinaf]

@westonized agreed

[pinaf]

@westonized np. a few more comments on test names and javadocs and we are good.

[westonal]

@pinaf Thanks again, applied comments, mostly as you said. Please re-review once more.

[pinaf]

@westonized good job!

[pinaf]

@rultor merge

[westonal]

@pinaf Thank you!

[rultor]

@rultor merge

@pinaf Thanks for your request. @yegor256 Please confirm this.

[westonal]

@yegor256 Please complete, I can't get any other tasks assigned for 4 days now, because both my tasks are waiting on your confirmation.

[westonal]

@yegor256 please complete this merge

[pinaf]

@yegor256 hello?

[yegor256]

@westonized hm... looks very strange. why do we need to keep both attributes and this multimap?? we just need ONE storage of attributes, in a multi map.

[westonal]

@yegor256 That is a larger task which I broke out with this todo in accordance with "we encourage developers to deliver unfinished components"

"@todo #31:30min Decide how this multimap should behave during Map methods
 +     *  This class implements Map<String, String> and how all values should
 +     *  behave after the many Map methods is unclear.

e.g. When I do this put("A","B") followed by get("A") I expect "B", but what if "A" already has other values in the multimap. And do multiple calls to put, overwrite, or add additional values?

Until that is decided, we need both. Or this would be so much simpler if the type was immutable and didn't implement Map<String, String>, and so didn't have to worry about the related potential liskov violations and edge cases.

Also note, this is not a case of a simple refactor to use this backing multimap, as none of the map functions are tested, so the only safe way to do this is by introducing a parallel field, and as a separate task, test all map functions before the refactor to use it. Had it been properly tested I would have attempted to use it without fear of breaking something.

[westonal]

@yegor256 please let me know what's going on with this? If it's not good enough let me know I'll just close it right now. But reading the issue, I'm not the first person to this conclusion. The only way you're going to get what you're after in a single 30 minute task, is to wait for a dev who doesn't know/care about liskov to come along.

[yegor256]

@westonized OK, I'm ready to accept this code, but only if you explain in the TODO more explicitly, that the existence of two storages is a temporary solution and is definitely a mistake, which we have to fix ASAP

[westonal]

@yegor256

The current design appears to have followed this line of thought, we have collected key-value pairs, so we might as well say this class implements Map, whether this is a useful thing or not. We will allow users to collect the values from their manifests and then add extra values by hand, again, whether this is useful or not.

This seemed OK, until the requirement to support both the Map<String, String> interface, but internally maintain a Map<String, List<String>>. Then things become blurred WRT Liskov as I have previously pointed out, i.e. you must carefully implement this so that this class respects the expected behavior of any other Map.

That's not impossible, however there are zero tests around the vast majority of Map<String, String> interface methods, such as size, isEmpty, containsValue, put, putAll, clear, keySet, values, entrySet, equals, hashCode, remove, replace etc. All implemented, but none covered by tests. This makes the refactor non-trivial, it requires that someone write the tests for all of these. And that is part of the task in the todo, the rest of the task is to convert each over to use the multi-map, but as you can see, there are far too many untested methods to test to fit in to this task. But there is a far easier alternative:

I suggest we do not implement Map<String, String> but instead have a method signature on the Manifest class or ideally an immutable structure; Map<String, String> asMap(), which yields from the immutable a writable copy that the user can do with what they wish and behind it a standard concrete Map.

[yegor256]

@westonized yes, no argument about that, but current implementation of the class is absolutely wrong. we can't live with it. we should either make our TODO explicitly screaming for refactoring or fix the class. up to you.

[westonal]

@yegor256 "we can't live with it" OK, how about we park this (marked blocked), do a new task first to refactor Manifests to an immutable with an asMap method, then I can easily resurrect this branch based on the newly immutable type, without the need for any todo.

[yegor256]

@westonized yes, it's a good idea, please submit a ticket

[pinaf]

@dmarkov we're waiting on #38 here

[dmarkov]

@dmarkov we're waiting on #38 here

@pinaf right, let's wait for #38

[0crat]

Oops! Job gh:jcabi/jcabi-manifests#35 was not in scope