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

Use lenses for JSON editing. Upgrade to hedgehog-extras0.5.1.0 #5626

Conversation

newhoggy
Copy link
Contributor

@newhoggy newhoggy commented Jan 6, 2024

Description

Upgrade to hedgehog-extras-0.5.1.0. Use lenses for JSON editing.

Checklist

  • Commit sequence broadly makes sense and commits have useful messages
  • New tests are added if needed and existing tests are updated. These may include:
    • golden tests
    • property tests
    • roundtrip tests
    • integration tests
      See Runnings tests for more details
  • Any changes are noted in the CHANGELOG.md for affected package
  • The version bounds in .cabal files are updated
  • CI passes. See note on CI. The following CI checks are required:
    • Code is linted with hlint. See .github/workflows/check-hlint.yml to get the hlint version
    • Code is formatted with stylish-haskell. See .github/workflows/stylish-haskell.yml to get the stylish-haskell version
    • Code builds on Linux, MacOS and Windows for ghc-8.10.7 and ghc-9.2.7
  • Self-reviewed the diff

Note on CI

If your PR is from a fork, the necessary CI jobs won't trigger automatically for security reasons.
You will need to get someone with write privileges. Please contact IOG node developers to do this
for you.

@newhoggy newhoggy force-pushed the newhoggy/upgrade-to-cardano-cli-8.17.0.0-and-cardano-api-8.37.0.0 branch from c767c8d to 04fc77d Compare January 6, 2024 04:51
& L.key "rho" . L._Double .~ 0.1
& L.key "tau" . L._Double .~ 0.1
& L.key "updateQuorum" . L._Integer .~ 2
& L.key "protocolParams" . L.key "protocolVersion" . L.key "major" . L._Integer .~ 8
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It turns out that lenses make a good compatibility shim. The aeson API keeps making breaking changes (for example switching from HashMap to KeyMap lenses help us having to deal with these breaking changes.

@newhoggy newhoggy marked this pull request as ready for review January 6, 2024 04:54
@newhoggy newhoggy force-pushed the newhoggy/upgrade-to-cardano-cli-8.17.0.0-and-cardano-api-8.37.0.0 branch from 04fc77d to 176cddf Compare January 8, 2024 09:25
@carbolymer
Copy link
Contributor

Kind of superceded by #5621 ?

Comment on lines +110 to +111
& L.key "rho" . L._Double .~ 0.1
& L.key "tau" . L._Double .~ 0.1
Copy link
Contributor

Choose a reason for hiding this comment

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

Those are wrong, rho and tau can't be at the top level.

I am going to steal your lenses idea and put it into my PR. 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Where should they go instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is not a regression in any case.

Copy link
Contributor

Choose a reason for hiding this comment

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

Inside protocolParams. This change just went overlooked, and I've stumbled upon it.

@newhoggy newhoggy force-pushed the newhoggy/upgrade-to-cardano-cli-8.17.0.0-and-cardano-api-8.37.0.0 branch 3 times, most recently from 85e6323 to 0db4af8 Compare January 12, 2024 13:46
@newhoggy newhoggy changed the title Upgrade to cardano-cli-8.17.0.0 and cardano-api-8.37.0.0 Use lenses for JSON editing. Upgrade to hedgehog-extras0.5.1.0 Jan 12, 2024
@newhoggy newhoggy added this pull request to the merge queue Jan 13, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Jan 13, 2024
@newhoggy newhoggy added this pull request to the merge queue Jan 13, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Jan 13, 2024
@newhoggy newhoggy force-pushed the newhoggy/upgrade-to-cardano-cli-8.17.0.0-and-cardano-api-8.37.0.0 branch from 0db4af8 to 28b8f0f Compare January 14, 2024 01:06
@newhoggy newhoggy added this pull request to the merge queue Jan 14, 2024
Merged via the queue into master with commit 639cf2f Jan 14, 2024
20 checks passed
@newhoggy newhoggy deleted the newhoggy/upgrade-to-cardano-cli-8.17.0.0-and-cardano-api-8.37.0.0 branch January 14, 2024 05:10
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

3 participants