Skip to content

Conversation

@bladyjoker
Copy link

@bladyjoker bladyjoker commented Apr 18, 2022

Hey @gnumonik, we have some goodies here I want you to check out:

  1. Testing works locally, not yet in CI (noisy Spago)
  2. Added code quality and assistance tools
  3. Nix
  4. CI
  5. ToData/FromData is still untested

@bladyjoker bladyjoker self-assigned this Apr 18, 2022
@bladyjoker bladyjoker requested a review from gnumonik April 18, 2022 15:39
| Enum
| Bounded
| HasConstrIndex
| ToData
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need this? I.e. do we really expect anyone using the library to want to generate HasConstrIndices instances but not To/FromData instances?

Either way I think mkSumTypeIndexed should generate ToData/FromData instances by default. Maybe we should rename it to mkPlutusSum or something like that?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I completely agree!

@gnumonik
Copy link
Collaborator

I think everything here is good. Going to merge it in a second.

@bladyjoker
Copy link
Author

@gnumonik that's it from me today, I didn't manage to generate the instance clauses so if you can, I'd be happy if you could continue with the following:

  • Turn the test green 1) buildBridge without lens-code-gen tests the generation of a CTL HasConstrIndices/ToData/FromData
  • Figure out flakiness of writePSTypesWith should produce aeson-compatible argonaut instances if doable
  • Merge this PR
  • Introduce the (the essential) changes you have pending in your existing PRs:
    • your comments and suggestions from here
    • everything needed to actually generate HasConstrIndices, ToData, FromData
  • Prepare a PR in Cardax to use the new version

We need to be able to continue user registration tomorrow, for that we need ToData/FromData instances.

Cheers,
Draz

Gregg Sean hunter added 2 commits April 18, 2022 15:34
…now. mkSumTypeIndexed + its extremelyUnsafe friend now generate Argonaut JSON instances by default. Test updated to account for those two changes. Test now passes.
@gnumonik
Copy link
Collaborator

Notes (mainly for my own benefit):

  • This should finally work the way it needs to for Cardax. We now generate ToData/FromData instances.

  • I'm punting on the Argonaut/spago2nix stuff for right now. The existing tests say the instances are argonaut compatible and I'm not that sure how to hook up spago2nix output to our flake. I'll come back to this later today if I have time.

  • I think these changes are sufficient for us to integrate into Cardax. Actually merging now.

@gnumonik gnumonik merged commit ce21505 into master Apr 18, 2022
This was referenced Apr 19, 2022
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.

3 participants