fix(cache): OnDemandType in no longer an enum #4673
Conversation
Plugins that wanted to add an OnDemandType previously wouldn't be able to add to the enum.
| public class OnDemandType { | ||
| private final String value; | ||
|
|
||
| public static OnDemandType fromString(String value) { |
There was a problem hiding this comment.
The enum had the property that for a given string s, fromString(s) always returned a reference to the same OnDemandType. Now a new object is created every time, so it is longer true that fromString("abc") == fromString("abc").
This will break any place where we compare OnDemandType objects using ==, which appears to be in a lot of places.
It's probably a lot easier/safer to update fromString to retain the prior behavior than to update all the usages.
There was a problem hiding this comment.
@ezimanyi Thank you, great catch! You are right. Where == is used in Java files it will do an instance comparison and fail. I'd like to discourage instance comparison because it would only work for the built in types that exist in the OnDemandType class. I looked through all the files and only found three cases where instance comparison happens so I just changed those.
There was a problem hiding this comment.
Thanks! I guess as one more thing I'd love to see a test on .equals to make sure it's behaving how it seems...looks correct but if we're now depending on that in more places would be good to have a test on this new function.
There was a problem hiding this comment.
@ezimanyi Added a plain java junit test as not to add an Groovy.
There was a problem hiding this comment.
Awesome, thanks! Yeah I've also stopped writing even tests in Groovy, +1 to Java unit tests.
Plugins that wanted to add an OnDemandType previously wouldn't be able to add to the enum.