search

pitt500 / SwiftAndTipsMacros

A list of Swift Macros to make your coding live on Apple ecosystem simpler and more productive.
MIT License
92 stars 7 forks source link

feat: SampleBuilderMacroForClass #3

Closed alirzaeram closed 8 months ago

alirzaeram commented 11 months ago

Add Class support for SampleBuilderMacro

pitt500 commented 11 months ago

Hi @alirzaeram,

Firstly, I apologize for the delay in responding to your message. Secondly, I'd like to thank you for the time and effort you've put into this PR.

I will review your code today and provide some feedback. Right off the bat, I've noticed that the PR does not include tests for the classes. Could you please review the tests written for structs and create similar tests for the classes?

Thank you.

Thanks.

alirzaeram commented 11 months ago

Hello @pitt500

Sure I'll add the similar tests for classes today and let you know to review the code,

Thank you

alirzaeram commented 11 months ago

Hi @pitt500 please review the changes

pitt500 commented 11 months ago

Hi @pitt500 please review the changes

Hi @alirzaeram , sorry for the delay. Let me review your PR today. Thanks!

pitt500 commented 11 months ago

I'm sorry for my delayed review and response @alirzaeram, I had a crazy week (and weekend) because I have a pending release. Anyway, I will do my best to reply to your PR during this week. My apologies

pitt500 commented 11 months ago

Thanks again for the effort driving this @alirzaeram! 🙌🏻

While I was reviewing your PR, I copy-pasted the content of your test "testSampleBuilderMacro_class_with_many_inits" into main.swift and I got an interesting issue:

Screenshot 2023-11-13 at 9 06 13

A TL;DR explanation of what is happening is here: https://forums.swift.org/t/why-error-covariant-self-type-cannot-be-referenced-from-a-stored-property-initializer/42235

Self cannot be guaranteed to be the same type as a regular struct because classes support inheritance. Actually, Due to inheritance, I'm not sure if would be a good idea to add this feature to SampleBuilder. We need to review all the tradeoffs and edge cases before proceeding, otherwise, we will break people's code.

If you are willing to continue with this exploration (considering that we may end up not adding this feature to SampleBuilder), I would suggest thinking about this:

  1. Should we restrict this feature to only final classes? (Although this will not fix the issue above)
  2. Should we just replace Self with the actual type's name? (Looks like this is fixing the issue, but it will impact Structs and Enums)
  3. Do we actually need support for Classes? Swift is pretty oriented to value types, especially for data models. (What would be the justification?)

And if we decide to move forward, I will suggest to do this:

Test the code using main.swift because the unit test is not catching semantic errors like the one above. Also, you can create a macro that is syntactically incorrect, but the test will pass because the unit test is not reviewing that the actual syntax is valid, just that the expected piece is code is the same as the actual (That's why the unit tests are not 100% reliable to make sure that macro will work correctly).

Let me know if you have any questions.

pitt500 commented 10 months ago

Hi @alirzaeram,

I wonder if you have any questions regarding the issue above 🤔

pitt500 commented 8 months ago

Hi @alirzaeram, It has been a while since this PR was created. Since I haven't received any feedback regarding my comment above, I will close this PR by the end of the next week if I don't get more comments from you.

You can reopen it at any time you want to continue this work. Thanks!

pitt500 commented 8 months ago

Closing this PR due to inactivity. Feel free to reopen it at any time. Thank you!