Skip to content
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

Aliases #866

Merged
merged 16 commits into from
Feb 1, 2023
Merged

Aliases #866

merged 16 commits into from
Feb 1, 2023

Conversation

erikvanoosten
Copy link
Contributor

@erikvanoosten erikvanoosten commented Jan 24, 2023

Add support for aliases by defining an annotation.
Solves #864.

Research into implementing aliases defined from an annotation. This does the minimum possible (only scala 2.x for now). Any hints on how to improve the idea are welcome.
@erikvanoosten erikvanoosten requested a review from a team as a code owner January 24, 2023 19:22
@erikvanoosten erikvanoosten marked this pull request as draft January 24, 2023 19:22
@fsvehla
Copy link
Contributor

fsvehla commented Jan 25, 2023

This looks promising. Can you lint the code so that we can see whether tests pass?

@erikvanoosten
Copy link
Contributor Author

@fsvehla The tests pass now. I will add some unit tests to see how the new feature works. I am particularly interested to see what error messages will look like.

@erikvanoosten
Copy link
Contributor Author

@fsvehla Can you take another look? As far as I can see it is complete now (except for Scala 3).

@fsvehla
Copy link
Contributor

fsvehla commented Jan 25, 2023

Is Scala 3 support something you can tackle?

@erikvanoosten
Copy link
Contributor Author

Is Scala 3 support something you can tackle?

Sure. Just not today anymore.
BTW, lets not forget documentation :)

@erikvanoosten erikvanoosten marked this pull request as ready for review January 26, 2023 20:23
@erikvanoosten
Copy link
Contributor Author

erikvanoosten commented Jan 26, 2023

@fsvehla I can't get the scala 3 tests to pass on my machine (tried with sbt ++3.2.1 testJVM). It seems as if sbt doesn't pick up my changes to the scala 3 macros at all. Any idea what I am doing wrong?

The StringMatrix unit tests now take quite a long time (~150 seconds on my machine). I think that the complex generator expression is to blame.

I wanted to add a unit test that asserts that code with an alias containing the name of an already defined field fails to compile. Something like:

assertFailsCompilation {
  case class Person(@jsonAliases("n") name: String, n: Int)
}

Any idea on how to do achieve this?

Otherwise, I think this feature is good to go. Any comments and feedback is welcome.

@erikvanoosten
Copy link
Contributor Author

I see some more tests are failing. Any help to fix those is appreciated.

@erikvanoosten erikvanoosten changed the title Aliases rfc Aliases Jan 27, 2023
@erikvanoosten
Copy link
Contributor Author

erikvanoosten commented Jan 28, 2023

In the end it was a silly mistake. The scala 3 tests pass now.

I would still like to introduce a test like:

"case class Person(@jsonAliases("n") name: String, n: Int)" shouldNot compile

I tried to use typeCheck but it doesn't work as I expected (see commented out tests).
We could also try scalatest's support (as in the example above). Is it okay if I pull in scalatest for this?

@erikvanoosten
Copy link
Contributor Author

@fsvehla could you start the build again please?

@erikvanoosten
Copy link
Contributor Author

erikvanoosten commented Jan 29, 2023

We could also try scalatest's support (as in the example above). Is it okay if I pull in scalatest for this?

I suspect that Magnolia doesn't actually evaluate the join method when the gen method is invoked. If that it is true, I do not see how we can improve this PR any further (except by removing the commented out tests).

@erikvanoosten
Copy link
Contributor Author

erikvanoosten commented Jan 30, 2023

@fsvehla All cleaned up and ready for a full review and merging...

@erikvanoosten
Copy link
Contributor Author

@fsvehla Sorry, I forgot to run the tests for scala 3 locally. Should be fixed now.

fsvehla
fsvehla previously approved these changes Jan 30, 2023
@erikvanoosten
Copy link
Contributor Author

Its not fixed though. Scala 3 uses different class names then scala 2. I'll have to be smart with the test assertions... I will fix it tomorrow.

@erikvanoosten
Copy link
Contributor Author

Okay, should be fine now for Scala 2 and 3 on the JVM. Let's see how all the different versions and platforms do...

@erikvanoosten
Copy link
Contributor Author

erikvanoosten commented Jan 31, 2023

Aaarrggh, now I forgot the formatting.
Fixed!

All other jobs are green! :)

@erikvanoosten
Copy link
Contributor Author

All green! 😄

@fsvehla
Copy link
Contributor

fsvehla commented Feb 1, 2023

Great work, thanks!

@fsvehla fsvehla merged commit 172a788 into zio:series/2.x Feb 1, 2023
@erikvanoosten erikvanoosten deleted the aliases branch February 1, 2023 12:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants