-
Notifications
You must be signed in to change notification settings - Fork 3.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Optimized mandatory attributes #31777
feat: Optimized mandatory attributes #31777
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
this(attributeList, attributeList.foldLeft(Map.empty[Class[_], Attributes.MandatoryAttribute]) { | ||
case (acc, attribute) => | ||
attribute match { | ||
case m: Attributes.MandatoryAttribute if !acc.contains(m.getClass) => acc.updated(m.getClass, m) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this the most efficient way to build the Map? Maybe it is because we have to use the first attribute in the list and then a MapBuilder can't be used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, but I have thought about maybe just reverse building it instead with a builder, that way the right value "wins" and lists should anyway mostly be short in this constructor.
It is not super important though, because for the default set of attributes we now invoke this just once when the materializer is created, rather than on each stream materialization like earlier, and then the and
-methods are used instead in actual attribute changes on the stream.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rewrote it with a reversed iteration instead anyway, please take a second look.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lol, already forgot I asked for a second look and merged. I'll do a follow up PR if you spot anything.
btw, shall we use the |
Looks like related test failures for |
Binary breaking changes, but very unlikely anyone is using those Scala 3 case class of 'Attributes'
Binary breaking changes, but very unlikely anyone is using those Scala 3 case class of 'Attributes'
No description provided.