Fix make macro #9147
Closed
Fix make macro #9147
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
kyri-petrou
reviewed
Aug 27, 2024
Comment on lines
+18
to
+19
| private def get(k: Key, m: List[Key]): Int = | ||
| m.count(key => areEquals(key, k)) |
There was a problem hiding this comment.
Perhaps we should add an exists method as well here just to avoid traversing the whole list in L84 below?
There was a problem hiding this comment.
Yes and also create another custom method because we only need to differentiate between 0, 1 and >1 appearances in the other cases
kyri-petrou
reviewed
Aug 27, 2024
| m.count(key => areEquals(key, k)) | ||
|
|
||
| private def areEquals(k1: Key, k2: Key): Boolean = | ||
| keyEquals(k1, k2) |
There was a problem hiding this comment.
Doesn't this needs to be keyEquals(k1, k2) || keyEquals(k2, k1)?
There was a problem hiding this comment.
It shouldn't be needed but the order is important, maybe that was an issue if you tried this version also
There was a problem hiding this comment.
Should probably have a test here to catch the "wrong" implementation.
|
After making this change and publishing locally, it seems the test compilation errors are gone, but now I'm getting a different compilation error in Scala 3 with a very large |
|
@pablf Would like to get this in. Can you re-open when conflicts are merged and it's no longer a draft?? 🙏 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Graphimplementation using aListinstead of aMapso that possible differences in how scala registers as keys the types are not a problem. I think this should be somewhat faster and could potentially use more memory if there is a lot of redundancy.@kyri-petrou, @RobVermazeren, @strokyl, I don't know if this fixes #9145 because I can't reproduce the issue. The tests for zio and zio-test were passing and I haven't been able to reproduce the issue locally with non-zio repositories. If someone of you could either open a repository with a working example of the issue so I can check what's happening or try this version locally, that would be really helpful!
Respect to the @butcherless issue, your error can only appear once the layer construction has been correct, so it isn't entirely related. I have added a fix for that. I don't know if it's the more appropriate as the layer construction is a bit more complex than before and can't be printed in such a nice way.