Feature/mixin quantifiers - #2643
Conversation
81a1c74 to
ffdb662
Compare
Earthcomputer
left a comment
Earthcomputer
left a comment
There was a problem hiding this comment.
I still need to test it, but reviewed the code
| override fun matchField(owner: String, name: String, desc: String): Boolean { | ||
| assert(!owner.contains('.')) | ||
| return (this.matchAllNames || this.name == name) && | ||
| return canEverMatch(name) && |
There was a problem hiding this comment.
Even though the implementation is the same, I'm not sure this is the right function to use here logically. Maybe however we can rename the function to matchesName or something
| override fun matchField(owner: String, name: String, desc: String): Boolean { | ||
| assert(!owner.contains('.')) | ||
| return (this.matchAllNames || this.name == name) && | ||
| return canEverMatch(name) && |
|
|
||
| val Sequence<*>.countIs get() = SequenceCountCompareOps(this) | ||
|
|
||
| class SequenceCountCompareOps(private val sequence: Sequence<*>) { |
There was a problem hiding this comment.
Not a big fan of magic syntax like this
There was a problem hiding this comment.
What would you suggest instead? It's a concept from Scala so it's at least somewhat established in that sense, but even aside from being nice to read I don't think there's any other way to expose the same functionality without 3 (or even 6) separate functions which feels overkill.
There was a problem hiding this comment.
I wasn't familiar with the pattern and since it's not a Scala codebase you can't necessarily expect the average reader to be familiar with the pattern. I was also confused at the usage site and had to read the implementation to understand what it was.
I reckon that unless this is commonly needed, reverting to the previous code .drop(1).any() is fine
There was a problem hiding this comment.
It's used in 2 other places too as .countIs >= minMatches or similar
There was a problem hiding this comment.
It reads quite well imo
There was a problem hiding this comment.
I read it as "count is" initially and I was like "what is i?". I'm just not expecting to read code in this way.
I think implementing them as separate functions is clearer. You can express all the inequalities as drop(?).any() so it's at least not too many lines
There was a problem hiding this comment.
Perhaps sizeIs would be clearer? Or would you rather .countIsAtLeast, .countIsMoreThan, etc?
There was a problem hiding this comment.
Yeah let's do countIsAtLeast
Mixin MemberInfo strings are only tangentially related to concrete MemberReferences. We also introduce a MemberMatcher superinterface for resolving things.
Really nothing is "ambiguous" (except perhaps to the reader), it's just that Mixin's default behaviour of matching the first method in a given class is unexpected, but I'll leave the terminology for now. Adding any explicit quantifier suppresses this inspection, including `{1}` if that's really what's desired.
Additionally, adding a wildcard is almost never the correct fix. Choosing one of the available descriptors is what should be offered instead, but I'll leave that too for now.
Causes targets not to show as unresolved when they should.
ffdb662 to
a0977d6
Compare
2 participants
Basic support for Mixin quantifiers, along with related bugfixes. Commits best consumed invididually.
Closes #2518