-
Notifications
You must be signed in to change notification settings - Fork 531
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
macro based Modifier for Record #567
Conversation
Working on implementing Select in terms of Modify. It will turn it into single implementation of all CRUD operations |
Introduced Crud typeclass responsible for crud operations on Record. It substitutes Updater, Modifier, Remover and Selector. Selector is not yet removed because Crud marco is not being expanded when spawned inside WitnessWith. |
Thanks! I don't think it will be possible to merge this as is ... there are many people depending on the current type classes so removing them in this way isn't possible. Alternative implementations of the current type classes are possible (with the proviso that we need to maintain binary compatibility within the 2.3.x series), so maybe you could move back to (some iteration of) what you had earlier for me to review? |
Would it be acceptable to
Thanks. |
I suggest making the smallest change that restores the original operations and binary compatibility whilst adding the new functionality you want so that I have something I can start to review. |
Oh, i see that point 2 is not an option. Will be back with updates upon restoring of binary compatibility. |
Like in #563, you can keep binary compatibility with non-implicit stubs along the new code. |
Yes looks neat , will go this way! |
@alexarchambault , your great advice helped me with JVM binary compatibility, now have you any ideas of how to fix ScalaJs tests? |
…h "WitnessWith") 2)Renamed Modifier to Crud
blocked by the following ScalaJs issue scala-js/scala-js#2307 |
Issue scala-js/scala-js#2307 is fixed by sjrd and should be available in next ScalaJs release(0.69?) |
Good job. I'll bump the Scala.js version as soon as it's available and we can proceed from there. |
All tests were made involving get, updated, updateWIth and remove operations simultaneously: #420 #486 |
I don't follow. Can you say what 1) and 2) are? |
It would be really helpful to have a couple of examples which are infeasibly slow to compile before this change, but which compile in an acceptable amount of time after it. |
I have edited my previous response, hope it is more clear now. |
Should I provide a repo with examples? |
That would be really helpful :-) |
Here it is : Currently depends on shapeless 2.3.1-snapshot(locally published branch that contains these changes) |
Thanks for that! :-) |
YW, pls notify me if you need something else |
I'm good for now. Give me a day or two ... |
Thanks ! |
I've tried out the test project and the speedups are great! Thank you! I would like you change the structure of this change before I merge it though. Rather than having a single type class which does the work for all the operations could you stick with the current structure (ie. one type class for each operation, the existing one for all but the add operation which is new) and have your macro materialize instances of those using the strategy you've adopted here. The benefit of this is particularly visible wrt update: if the existing |
Great, thanks for your feedback! Please correct me if I am wrong: 1)The task is to materialize different type classes(having different method signatures) with a single macro(if it is not the case then would you pls be so kind to provide a bit more formal task definition ). 2)"replace" semantics differs from "updated" as it only supports "replace" operation ("updated" also acts as "add") so the code using "updated" method won't compile in number of places in case of drop-in replacement. Thanks! |
Apologies about the misunderstanding wrt replace vs. update ... yes, a separate replace method makes sense. Am I right that you're adding two fundamentally new operations on records: replace (which is like the existing If that's the case I would prefer to see those two operations added as independent type classes (eg. Each of these type classes requires a separate implicit macro materializer method, but those can delegate to a single shared method which contains the bulk of the implementation. As well as that, I'd like to see the method You've done all the hard work already, I'm really just proposing shuffling things around a bit ... I hope you don't mind. |
1)Regarding Update and Add: yes you are right. |
The requested implementations are provided. I apologize in advance in case if these questions are inappropriate, I might have overlooked some details which in this case I am asking you to point out. Thanks! |
Hmm ... that didn't come out quite the way I was expecting. Why have you added the |
I may have misunderstood you. Will try to do better today |
…ow to encode type members to avoid casting to Aux)
Could you pls have a quick glance on this branch again. |
|
||
//TODO: remove @noinline after switching to ScalaJs 0.69+ | ||
@noinline | ||
def unsafeCrud(l: HList, i: Int, f: Any => Any, remove:Boolean): (Any, HList) = { |
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.
I would really prefer this to be called unsafeUpdateWith
.
1)got it |
First, my sincere apologies for taking so long to get back to you on this. On 2) I think it would be better to go with |
Hi! |
No worries! :-) |
I've just pushed this which is partly inspired by the work you did on this PR ... many thanks for that. Some of the changes are cosmetic (eg. I don't particularly like the "crud" terminology). There were some technical issues as well. You were creating anonymous subclasses in your macros rather than instantiating a concrete class ... that can result in the creation of many more class files. I also factored your very general crud method into several simpler methods, this makes the use of On the additional operations, I added The main thing is that your compile time benchmark project now compiles in a sensible amount of time ... so thank you very much for encouraging me down this route, and I do hope you don't mind me hijacking this PR. |
Great news!! |
1)implemented macro based Modifier
2)implemented Updater and Remover in terms of Modifier
3)fixed the following inconsistency in record syntax:
before: "updated" method was used to update and add new fields and it was possible to occasionally add another field with same key(in case of providing wrong value type in updated method).
now : "updated" method only updates the field if the record contains appropriate key and value types, "add" method adds new field if record does not contain the field with same key.
IMPORTANT NOTE:
This change breaks binary compatibility with 2.3.0.