Optimize SystemEnvironmentPropertyMapper #21523
Conversation
spring-projects-issues
added
the
status: waiting-for-triage
snicoll
added
type: enhancement
| try { | ||
| return ConfigurationPropertyName.of(legacyCompatibleName).isAncestorOf(candidate); | ||
| } | ||
| catch (Exception ex) { |
There was a problem hiding this comment.
Can you share what that extra catch block is supposed to cover?
There was a problem hiding this comment.
The removal of isValid here might lead to exceptions if the name can't be constructed.
There was a problem hiding this comment.
I'd argue that legacyCompatibleName is a bit misleading then. It makes me wonder looking at the code why a catch block is necessary. Thoughts?
There was a problem hiding this comment.
I didn't change anything in that regards. I think the name was chosen to reflect the "legacy" format and isValid() was added to be sure that no incompatible output breaks the code here. This PR just sticks to that behaviour of not breaking anything with a potentially invalid name.
Having that said, I too had a hard time seeing how an invalid name can be produced from a valid name, but didn't want to change the intention of the code.
There was a problem hiding this comment.
Alright, thanks for the feedback. Flagging for team attention to see what the rest of the team thinks.
snicoll
added
the
for: team-attention
philwebb
removed
the
for: team-attention
Update `SystemEnvironmentPropertyMapper` to use single chars rather than strings whenever possible. See gh-21523
Introduce a new `ConfigurationPropertyName.ofIfValid` method to save us needing to throw and catch an exception unnecessarily. See gh-21523
|
Thanks once again @dreis2211! I split this up into a few smaller commits and also added a new method to |
Hi,
I just noticed a couple of minor performance tweaks in
SystemEnvironmentPropertyMapper. The biggest one is probably to avoid creatingConfigurationPropertyNameinstances twice for everyisAncestorOfcheck (once for the normalof()call and once for theisValid()check).Cheers,
Christoph