fix: put should return previous value instead of Optional #2595
Conversation
metacosm
commented
Nov 19, 2024
metacosm
commented
- refactor: add unit tests for DefaultManagedDependentResourceContext
- refactor: add putOrRemove to DefaultManagedDependentResourceContext
- refactor: deprecate ManagedDependentResourceContext#put
- fix: implementation of put should return value instead of Optional
Signed-off-by: Robert Young <robeyoun@redhat.com>
The javadoc of the existing put method states that it returns Optional. The implementation also returns Optional, but the method signature declares it returns T. Meaning the caller has to cast it to Optional to get at the return value. This new method will supercede the existing put method Signed-off-by: Robert Young <robeyoun@redhat.com>
The javadoc declares it returns Optional and the implementation returns Optional, but the method declares it returns T. put is replaced by putOrRemove with a consistent signature. Signed-off-by: Robert Young <robeyoun@redhat.com>
Signed-off-by: Chris Laprun <claprun@redhat.com>
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
Extension to #2587 to properly (according to me) fix the implementation to properly return the previous value as the implementation was done instead of returning |
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
|
Obsolete, was done as part of #2587 |
| return (T) previous; | ||
| } | ||
|
|
||
| // only for testing purposes |
There was a problem hiding this comment.
Seems this is not only used in testing.
There was a problem hiding this comment.
The whole purpose of this method is to be overridden in the test so that we can check that the logging really happens without having to set up a complex logging fixture just for that purpose.
| if (value == null) { | ||
| return (T) Optional.ofNullable(attributes.remove(key)); | ||
| return (T) attributes.remove(key); |
There was a problem hiding this comment.
IMHO, there should be a reset method. This seems unintuitive to me.
There was a problem hiding this comment.
agree, or simply a remove.
There was a problem hiding this comment.
Was debating that, indeed. Not sure how many people would actually need this so decided against it (people would usually put stuff in the context and get it back instead of removing them so I think this is an edge case).
There was a problem hiding this comment.
That is true, that the scope of this is the actual reconciliation. So maybe we don't even need remove at all. But if we do would be nice to have it explicit.
| @Test | ||
| void putNewValueThrowsExceptionIfTypesDiffer() { | ||
| // to check that we properly log things without setting up a complex fixture | ||
| final String[] messages = new String[1]; |
There was a problem hiding this comment.
This doesn't need to be an array.
There was a problem hiding this comment.
Unless I am mistaken, it does. Simply using a final String doesn't work since there is no way for Java to check that it is always assigned and therefore complains that you're assigning a value to a final variable in the overridden method. Not using a final String won't work either because this is effectively a capture so the variable needs to be final so the only way that I know of to do this is to use a final array and change the element.
