quicktype icon indicating copy to clipboard operation
quicktype copied to clipboard

Scala3 Support

Open Quafadas opened this issue 3 years ago • 5 comments

This is (most definitely!) a work in progress.

I have a use case where I wish to be able to parse the vega schema into scala3 case classses. This app looks like the most promising starting point for me.

As a starting point, I simply duplicated the Kotlin Type Only flavour and replaced some trivial syntax to make it scala like. It turns out, that kotlin isn't so far from scala :-)...

This PR is basically opened to see if there is any interest in merging such an effort, and to find out what would need to be done. This looks like a relatively large project, so I doubt I can get this to right place without guidance.

Anyone willing to point me in the right direction?

Quafadas avatar Apr 21 '22 13:04 Quafadas

So I believe this now produces syntactically valid scala code.

It does appear however, to have stumbled on a giant gotcha, that being that the JSON libraries I am aware of, actually do not (currently) support (the new) intersection types. i.e. these types cannot be de/serialised. This is a fairly obvious disadvantage of encoding them as such :-).

Oooof. TBC...

Quafadas avatar Apr 24 '22 20:04 Quafadas

At this stage, I'm happy with the implementation. As far as I can tell, it is passing many of the tests and producing code which meaningful.

Tests now fail primarily, because the ordering of the parameters is different in the input to output - I'm unclear how I can control this and don't really consider it a "bug" per se.

At this stage, I would need guidance on what would need to be done, in order to have this merged? IS anyone willing?

Quafadas avatar May 01 '22 22:05 Quafadas

For me, this command

FIXTURE=scala3 npm test

now passes 203 tests. The exluded tests contain things that I think will not work well in scala for example "_" as a variable name.

it appears to successfully generate types and circe encoders for the Vega/-lite schemas which is my use case, so I am now happy and will down tools.

Quafadas avatar May 02 '22 07:05 Quafadas

I'm aware of two niggling issues, which could be solved a some future stage;

  1. need a null type. type NullValue = None.type for unions, currently just putting that in manually.
  2. unions with duplicate shapes type A = Double | Stringand type B = Double | String generate conflicting givens. Needed to go through commenting some out, took 3 mins for Vega/Lite, so I think these are minor issues.

Quafadas avatar May 02 '22 14:05 Quafadas

I'm not aware of any outstanding problems with this now.

As far as I can tell, it produces types, encoders and decoders for the Vega / Lite schemas for scala 3 with circe...

Downing tools.

Quafadas avatar May 11 '22 12:05 Quafadas

@schani I saw you became active in this report again briefly… Want to see if you would have any hints on whether / there is the possibility to get this merged?

Quafadas avatar Oct 11 '22 05:10 Quafadas

@Quafadas our CI is back online, and I have updated your branch. You can see where you have build failures.

Please add your test fixture to test-pr.yaml to include it in CI.

dvdsgl avatar Jan 07 '23 22:01 dvdsgl

@Quafadas thanks! Once you resolve conflicts, I can approve you to run in CI.

dvdsgl avatar Jan 08 '23 17:01 dvdsgl

@dvdsg Thanks for looking at this :-).

I managed to get the CI responding on my fork - but it is as I think you have seen - alas, borked.

My plan would be to drop you a message if / when I manage to de-bork it... please bear with me.

Quafadas avatar Jan 08 '23 18:01 Quafadas

@dvdsgl HI David, I'd expect this to fail right now.

https://github.com/VirtusLab/scala-cli/issues/1797

I think we might need a release of Scala-cli... but it's very close!

Quafadas avatar Jan 19 '23 18:01 Quafadas

@Quafadas what's the latest?

dvdsgl avatar Jan 31 '23 05:01 dvdsgl

@dvdsgl Well, the Scala-cli team have been great. They seem to have solved the problem their side, and there's a fork of my fork, with the scala tests running green in CI; https://github.com/lwronski/quicktype/actions/runs/4015980810/jobs/6898402205

I would now wish to wait for them to release (I think it's planned for early feb)... which I think would be the last "blocker" from my perspective to believe this might be ready to merge. Let's assume the CI on my branch goes green once it's out...

To note;

  • At this stage the git history is a mess. I tried to squash it locally but there are merges and all sorts in the the history... so it is (to my skills) unsquashable. Depending on whether the commit history is important to you, I may have to start from a fresh branch off master?
  • When I looked through this again I realised at that some point, I wasn't sure this would get merged. The branch kind of turned into "stuff Simon wants to mess around with in quicktype" rather than it's original focus. Which means there is some cruft mixed in. Some of that cruft included generating "smithy" too. It's actually kind of useful and I've used it manually, but it currently doesn't go through the test suite. I think there is a pathway to that, but I don't think it's trivial, and I have no intent of doing it as part of this PR.

I'm trying to be transparent about the fact that this is messier than I wanted originally. If you're relaxed about these things, that's great for me and I dot'n think it's a "problem" per se to merge them. If that sounds problematic for whatever reason, I'll have to go through and prune some stuff I guess.

Quafadas avatar Jan 31 '23 06:01 Quafadas

@dvdsgl scala3 CI on my branch is now green and "correctly" setup - no nasty tricks or pre release software etc.

https://github.com/Quafadas/quicktype/actions/runs/4243439899

I need to sit down and unify it with the existing master branch (add back test for all other languages etc), but a heads up that I hope to ask for some headspace in the next week or so ... hope that's tractable and sorry for the delay.

Quafadas avatar Feb 22 '23 15:02 Quafadas

@Quafadas amazing! Looks like CI passed.

Can you please address these issues? They are mostly easy changes: https://deepsource.io/gh/quicktype/quicktype/run/fa2cdf69-2f4f-4a58-84a3-2519e0577cd1/javascript

dvdsgl avatar Feb 22 '23 17:02 dvdsgl

@dvdsgl I've looked at these as best I can and fixed where I'm able. Of this list;

Detected usage of the any type: I don't know how to fix this. I cross checked and this pattern is used in both Python and Kotlin renderers.

Duplicate case Labels: Removed and resolved

Default parameters placed after non defult: Resolved

Explicit type declarations : I think I got them all

Avoid using console : Removed these useages

Use const variables for variables which are never assiged : done

Require template literals instead of string concatenation : This I didn't do. There are places where some sort of implicit string conversion seems to be taking place and I'm not really sure how typescript would respond - my typescript foo is weak. Sorting these out would be rather time consuming. If I?m honest not super convinced about the benefit here either.

Class methods should utilise this : I had trouble with this; my view is that the rule does not understand the semantics of the program. In the example of "makeEnumCaseNamer", making that method static will cause the Scala3Renderer class to not compile, because it does not implement its interface. I claim fixing this would therefore require changing the ConvenienceRenderer interface... but if another language were in fact to require the "this" keyword, then we have a paradox. I'd claim this is out of scope.

Found shorthand type coercions : I think I got this one.

I've put back the GitHub workflows so that all the other languages will run too, and it only runs on PR to the master branch...

Quafadas avatar Feb 23 '23 21:02 Quafadas

Way to go @Quafadas!

CleanShot 2023-02-23 at 15 11 46@2x

dvdsgl avatar Feb 23 '23 23:02 dvdsgl

Looks like there is room for improvement on naming, but happy to accept now. Congratulations and thanks again!

dvdsgl avatar Feb 23 '23 23:02 dvdsgl

Shoot, the scala3 fixture failed but GitHub merged anyway. I am reverting in https://github.com/quicktype/quicktype/pull/2188

@Quafadas we're so close! Please reopen a new PR and let's see what's up with the tests.

dvdsgl avatar Feb 23 '23 23:02 dvdsgl

Um... whoops. I left in a + when trying to change to the suggested string literal syntax. Praise be for testing :-) ... new PR incoming.

Quafadas avatar Feb 24 '23 08:02 Quafadas