Fix MP Config TCK tests as part of #4516 #5050
Conversation
| @@ -72,7 +72,7 @@ public static String expandValue(String value, Cache cache) { | |||
| if (value == null) | |||
| return null; | |||
| final Expression compiled = cache.exprCache.computeIfAbsent(value, | |||
| str -> Expression.compile(str, Expression.Flag.LENIENT_SYNTAX)); | |||
| str -> Expression.compile(str, Expression.Flag.LENIENT_SYNTAX, Expression.Flag.NO_TRIM)); | |||
There was a problem hiding this comment.
Hum. Can't this potentially break a lot of applications? E.g. any application having a space at the end of a config line?
I'm not sure I would do it that last minute just to have some more TCK tests passing.
There was a problem hiding this comment.
Possibly, but if we want the change isn't it better to have it for 1.0 rather than after? As then it's even more of a breaking change
The question is whether we expect users to want leading/trailing spaces in configuration values. If they don't, or we don't think it makes sense, then we should include this change.
If we'd rather allow spaces and trim them, we can look to modify the TCK test to ignore leading/trailing spaces.
There was a problem hiding this comment.
I'm a bit torn on this. There might be a few specific config property where you would like spaces to be kept (a separator for instance) but for most of them, you don't really care and it might be a real pain.
I think it's worth a discussion on the list rather than sneaking it in like that.
There was a problem hiding this comment.
No this won't affect most cases. If anything it removes a behavioral breakage. And even if it did affect someone somehow, the error message would be very obvious: unlike the opposite case you'd get when you expect to preserve whitespace and it doesn't happen, which will be subtle and hard to detect.
|
Looks like we might have our first issue reported related to this #5305 |
No description provided.